code-423n4 / 2021-06-pooltogether-findings

0 stars 0 forks source link

`YieldSourcePrizePool_canAwardExternal` does not work #92

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

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.

asselstine commented 3 years ago

This is an interesting one:

Since yield sources are audited and analyzed, I think this is a pretty low risk. Additionally, not all of the yield sources are tokenized (Badger and Sushi are not), so it isn't a risk for them.

We could have canAwardExternal on the yield source itself, but it would add gas overhead.

aodhgan commented 3 years ago

Could we add an check - function _canAwardExternal(address _externalToken) internal override view returns (bool) { return _externalToken != address(yieldSource) && _externalToken != address(yieldSource.depositToken()) }

asselstine commented 3 years ago

We could add another check, but it's still arbitrary. The point is that the yield source knows what token the prize pool may or may not hold, so without asking the yield source it's just a guess.

Let's leave it as-is