PoolTogether — Sushi and Yearn V2 Yield Sources

Description

During the deposit in the supplyTokenTo function, the token transfer is happening after the shares are minted and before tokens are deposited to the yearn vault:

code/pooltogether-yearnv2-yield-source/contracts/yield-source/YearnV2YieldSource.sol:L117-L128

function supplyTokenTo(uint256 _amount, address to) override external {
    uint256 shares = _tokenToShares(_amount);

    _mint(to, shares);

    // NOTE: we have to deposit after calculating shares to mint
    token.safeTransferFrom(msg.sender, address(this), _amount);

    _depositInVault();

    emit SuppliedTokenTo(msg.sender, shares, _amount, to);
}

If the token allows the re-entrancy (e.g., ERC-777), the attacker can do one more transaction during the token transfer and call the supplyTokenTo function again. This second call will be done with already modified shares from the first deposit but non-modified token balances. That will lead to an increased amount of shares minted during the supplyTokenTo. By using that technique, it’s possible to steal funds from other users of the contract.

Recommendation

Have the re-entrancy guard on all the external functions.

Description

The deposit is usually made with all the token balance of the contract:

code/pooltogether-yearnv2-yield-source/contracts/yield-source/YearnV2YieldSource.sol:L171-L172

// this will deposit full balance (for cases like not enough room in Vault)
return v.deposit();

The Yearn vault contract has a limit of how many tokens can be deposited there. If the deposit hits the limit, only part of the tokens is deposited (not to exceed the limit). That case is not handled properly, the shares are minted as if all the tokens are accepted, and the “change” is not transferred back to the caller:

code/pooltogether-yearnv2-yield-source/contracts/yield-source/YearnV2YieldSource.sol:L117-L128

function supplyTokenTo(uint256 _amount, address to) override external {
    uint256 shares = _tokenToShares(_amount);

    _mint(to, shares);

    // NOTE: we have to deposit after calculating shares to mint
    token.safeTransferFrom(msg.sender, address(this), _amount);

    _depositInVault();

    emit SuppliedTokenTo(msg.sender, shares, _amount, to);
}

Recommendation

Handle the edge cases properly.

Description

The redeemToken function takes as argument the amount of SUSHI to redeem. Because the SushiBar’s leave function – which has to be called to achieve this goal – takes an amount of xSUSHI that is to be burned in exchange for SUSHI, redeemToken has to compute the amount of xSUSHI that will result in a return of as many SUSHI tokens as were requested.

code/sushi-pooltogether/contracts/SushiYieldSource.sol:L62-L87

/// @notice Redeems tokens from the yield source from the msg.sender, it burn yield bearing tokens and return token to the sender.
/// @param amount The amount of `token()` to withdraw.  Denominated in `token()` as above.
/// @return The actual amount of tokens that were redeemed.
function redeemToken(uint256 amount) public override returns (uint256) {
    ISushiBar bar = ISushiBar(sushiBar);
    ISushi sushi = ISushi(sushiAddr);

    uint256 totalShares = bar.totalSupply();
    uint256 barSushiBalance = sushi.balanceOf(address(bar));
    uint256 requiredShares = amount.mul(totalShares).div(barSushiBalance);

    uint256 barBeforeBalance = bar.balanceOf(address(this));
    uint256 sushiBeforeBalance = sushi.balanceOf(address(this));

    bar.leave(requiredShares);

    uint256 barAfterBalance = bar.balanceOf(address(this));
    uint256 sushiAfterBalance = sushi.balanceOf(address(this));

    uint256 barBalanceDiff = barBeforeBalance.sub(barAfterBalance);
    uint256 sushiBalanceDiff = sushiAfterBalance.sub(sushiBeforeBalance);

    balances[msg.sender] = balances[msg.sender].sub(barBalanceDiff);
    sushi.transfer(msg.sender, sushiBalanceDiff);
    return (sushiBalanceDiff);
}

Because the necessary calculations involve division and amounts have to be integral values, it is usually not possible to get the exact amount of SUSHI tokens that were requested. More precisely, let a denote the total supply of xSUSHI and b the SushiBar’s balance of SUSHI at a certain point in time. If the SushiBar’s leave function is supplied with x xSUSHI, then it will transfer floor(x * b / a) SUSHI. (We assume throughout this discussion that the numbers involved are small enough such that no overflow occurs and that a and b are not zero.)
Hence, if y is the amount of SUSHI requested, it would make sense to call leave with the biggest number x that satisfies floor(x * b / a) <= y or the smallest number x that satisfies floor(x * b / a) >= y. Which of the two is “better” or “correct” needs to be specified, based on the requirements of the caller of redeemToken. It seems plausible, though, that the first variant is the one that makes more sense in this context, and the current implementation of redeemToken supports this hypothesis. It calls leave with x1 := floor(y * a / b), which gives us floor(x1 * b / a) <= y. However, x1 is not necessarily the biggest number that satisfies the relation, so the caller of redeemToken might end up with less SUSHI than they could have gotten while still not exceeding y.

The correct amount to call leave with isx2 := floor((y * a + a - 1) / b) = max { x | floor(x * b / a) <= y }. Since |x2 - x1| <= 1, the difference in SUSHI is at most floor(b / a). Nevertheless, even this small difference might subvert fairly reasonable expectations. For example, if someone queries balanceOfToken and immediately after that feeds the result into redeemToken, they might very well expect to redeem exactly the given amount and not less; it’s their current balance, after all. However, that’s not always the case with the current implementation.

Recommendation

Calculate requiredShares based on the formula above (x2). We also recommend dealing in a clean way with the special cases totalShares == 0 and barSushiBalance == 0.

Description

The balanceOfToken computation is too pessimistic, i.e., it can underestimate the current balance slightly.

code/sushi-pooltogether/contracts/SushiYieldSource.sol:L29-L45

/// @notice Returns the total balance (in asset tokens).  This includes the deposits and interest.
/// @return The underlying balance of asset tokens
function balanceOfToken(address addr) public override returns (uint256) {
    if (balances[addr] == 0) return 0;
    ISushiBar bar = ISushiBar(sushiBar);

    uint256 shares = bar.balanceOf(address(this));
    uint256 totalShares = bar.totalSupply();

    uint256 sushiBalance =
        shares.mul(ISushi(sushiAddr).balanceOf(address(sushiBar))).div(
            totalShares
        );
    uint256 sourceShares = bar.balanceOf(address(this));

    return (balances[addr].mul(sushiBalance).div(sourceShares));
}

First, it calculates the amount of SUSHI that “belongs to” the yield source contract (sushiBalance), and then it determines the fraction of that amount that would be owed to the address in question. However, the “belongs to” above is a purely theoretical concept; it never happens that the yield source contract as a whole redeems and then distributes that amount among its shareholders; instead, if a shareholder redeems tokens, their request is passed through to the SushiBar. So in reality, there’s no reason for this two-step process, and the holder’s balance of SUSHI is more accurately computed as balances[addr].mul(ISushi(sushiAddr).balanceOf(address(sushiBar))).div(totalShares), which can be greater than what balanceOfToken currently returns. Note that this is the amount of SUSHI that addr could withdraw directly from the SushiBar, based on their amount of shares. Observe also that if we sum these numbers up over all holders in the yield source contract, the result is smaller than or equal to sushiBalance. So the sum still doesn’t exceed what “belongs to” the yield source contract.

Recommendation

The balanceOfToken function should use the formula above.

Description

The approval for token transfer is done in the following way:

code/pooltogether-yearnv2-yield-source/contracts/yield-source/YearnV2YieldSource.sol:L167-L170

if(token.allowance(address(this), address(v)) < token.balanceOf(address(this))) {
    token.safeApprove(address(v), 0);
    token.safeApprove(address(v), type(uint256).max);
}

Since the approval will be equal to the maximum value, there’s no need to make zero-value approval first.

Recommendation

Change two safeApprove to one regular approve with the maximum value.

Description

The state variables sushiBar and sushiAddr are initialized in the contract’s constructor and never changed afterward.

code/sushi-pooltogether/contracts/SushiYieldSource.sol:L12-L21

contract SushiYieldSource is IYieldSource {
    using SafeMath for uint256;
    address public sushiBar;
    address public sushiAddr;
    mapping(address => uint256) public balances;

    constructor(address _sushiBar, address _sushiAddr) public {
        sushiBar = _sushiBar;
        sushiAddr = _sushiAddr;
    }

They should be immutable; that would save some gas and make it clear that they won’t (and can’t) be changed once the contract has been deployed.
Moreover, they would better have more specific interface types than address, i.e., ISushiBar for sushiBar and ISushi for sushiAddr. That would be safer and make the code more readable.

Recommendation

Make these two state variables immutable and change their types as indicated above. Remove the corresponding explicit type conversions in the rest of the contract, and add explicit conversions to type address where necessary.

Description

In function redeemToken, barBalanceDiff is always the same as requiredShares because the SushiBar’s leave function burns exactly requiredShares xSUSHI.

code/sushi-pooltogether/contracts/SushiYieldSource.sol:L73-L84

uint256 barBeforeBalance = bar.balanceOf(address(this));
uint256 sushiBeforeBalance = sushi.balanceOf(address(this));

bar.leave(requiredShares);

uint256 barAfterBalance = bar.balanceOf(address(this));
uint256 sushiAfterBalance = sushi.balanceOf(address(this));

uint256 barBalanceDiff = barBeforeBalance.sub(barAfterBalance);
uint256 sushiBalanceDiff = sushiAfterBalance.sub(sushiBeforeBalance);

balances[msg.sender] = balances[msg.sender].sub(barBalanceDiff);

Recommendation

Use requiredShares instead of barBalanceDiff, and remove the unnecessary queries and variables.

Description

The ISushiBar interface declares a transfer function.

code/sushi-pooltogether/contracts/ISushiBar.sol:L5-L17

interface ISushiBar {
    function enter(uint256 _amount) external;

    function leave(uint256 _share) external;

    function totalSupply() external view returns (uint256);

    function balanceOf(address account) external view returns (uint256);

    function transfer(address recipient, uint256 amount)
        external
        returns (bool);
}

However, this function is never used, so it could be removed from the interface. Other functions that the SushiBar provides but are not used (approve, for example) aren’t part of the interface either.

Recommendation

Remove the transfer declaration from the ISushiBar interface.