PoolTogether contest - cmichel's results

Handle

cmichel

Vulnerability details

YearnV2YieldSource._withdrawFromVault uses a wrong subtraction. When withdrawing from the vault one redeems yTokens for tokens, thus the token balance of the contract should increase after withdrawal. But the contract subtracts the currentBalance from the previousBalance:

uint256 yShares = _tokenToYShares(amount);
uint256 previousBalance = token.balanceOf(address(this));
// we accept losses to avoid being locked in the Vault (if losses happened for some reason)
if(maxLosses != 0) {
    vault.withdraw(yShares, address(this), maxLosses);
} else {
    vault.withdraw(yShares);
}
uint256 currentBalance = token.balanceOf(address(this));
// @audit-issue this seems wrong
return previousBalance.sub(currentBalance);

Impact

All vault withdrawals fail due to the integer underflow as the previousBalance is less than currentBalance. Users won't be able to get back their investment.

Recommended Mitigation Steps

It should return currentBalance > previousBalance ? currentBalance - previousBalance : 0

Handle

cmichel

Vulnerability details

When suppling to the BadgerYieldSource, some amount of badger is deposited to badgerSett and one receives badgerSett share tokens in return which are stored in the balances mapping of the user. So far this is correct.

The balanceOfToken function should then return the redeemable balance in badger for the user's badgerSett balance. It computes it as the pro-rata share of the user balance (compared to the total-supply of badgerSett) on the badger in the vault:

balances[addr].mul(
  badger.balanceOf(address(badgerSett))
).div(
  badgerSett.totalSupply()
)

However, badger.balanceOf(address(badgerSett)) is only a small amount of badger that is deployed in the vault ("Sett") due to most of the capital being deployed to the strategies. Therefore, it under-reports the actual balance:

Typically, a Sett will keep a small portion of deposited funds in reserve to handle small withdrawals cheaply. Badger Docs

Impact

Any contract or user calling the balanceOf function will receive a value that is far lower than the actual balance. Using this value as a basis for computations will lead to further errors in the integrations.

Recommended Mitigation Steps

It should use badgerSett.balance() instead of badger.balanceOf(address(badgerSett)) to also account for "the balance in the Sett, the Controller, and the Strategy".

Handle

cmichel

Vulnerability details

One can withdraw the entire PrizePool deposit by circumventing the timelock. Assume the user has no credits for ease of computation:

• user calls withdrawWithTimelockFrom(user, amount=userBalance) with their entire balance. This "mints" an equivalent amount of timelock and resets _unlockTimestamps[user] = timestamp = blockTime + lockDuration.
• user calls withdrawWithTimelockFrom(user, amount=0) again but this time withdrawing 0 amount. This will return a lockDuration of 0 and thus unlockTimestamp = blockTime. The inner _mintTimelock now resets _unlockTimestamps[user] = unlockTimestamp
• As if (timestamp <= _currentTime()) is true, the full users amount is now transferred out to the user in the _sweepTimelockBalances call.

Impact

Users don't need to wait for their deposit to contribute their fair share to the prize pool. They can join before the awards and leave right after without a penalty which leads to significant issues for the protocol. It's the superior strategy but it leads to no investments in the strategy to earn the actual interest.

Recommended Mitigation Steps

The unlock timestamp should be increased by duration each time, instead of being reset to the duration.

Handle

cmichel

Vulnerability details

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

It is not checked in BadgerYieldSource.supplyTokenTo.

Impact

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer.

As the badger is merely a parameter to the yield source it is not known which token & Sett will end up actually being used.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

BadgerYieldSource.redeemToken: no usage of SafeMath can lead to overflows here as the amount parameter is chosen by the attacker.

amount.mul(totalShares) + totalShares

Impact

It does most likely not have an impact, we still recommend using SafeMath.

Recommended Mitigation Steps

Use SafeMath.

Handle

cmichel

Vulnerability details

The idea of YieldSourcePrizePool_canAwardExternal seems to be to disallow awarding the interest-bearing token of the yield source, like aTokens, cTokens, yTokens.

"@dev Different yield sources will hold the deposits as another kind of token: such a Compound's cToken. The prize strategy should not be allowed to move those tokens."

However, the code checks _externalToken != address(yieldSource) where yieldSource is the actual yield strategy contract and not the strategy's interest-bearing token. Note that the yieldSource is usually not even a token contract except for ATokenYieldSource and YearnV2YieldSource.

Impact

The _canAwardExternal does not work as expected. It might be possible to award the interest-bearing token which would lead to errors and loss of funds when trying to redeem underlying.

Recommended Mitigation Steps

There doesn't seem to be a function to return the interest-bearing token. It needs to be added, similar to depositToken() which retrieves the underlying token.