Platform: Code4rena
Start Date: 17/06/2021
End Date: 23/06/2021
Period: 7 days
Status: Completed
Reporters: moneylegobatman, ninek
Pot Size: $60,000 USDC
Participants: 12
Reporters: moneylegobatman, ninek
Judge: LSDan
Id: 14
League: ETH
Auditor per page
The code under audit is listed below in the scope, but here are links to the containing projects for ease of auditing:
PoolTogether is a protocol that gamifies interest. Users deposit funds into a "Prize Pool". The Prize Pool uses a Yield Source to generate interest. The interest is then exposed to a Prize Strategy, which distributes the interest as prizes. For a thorough introduction to the protocol, please refer to the Protocol Overview developer documentation.
The scope of this contest includes the PoolTogether Protocol contracts that escrow funds. We want wardens to focus exclusively on the security of user deposits. The PoolTogether Protocol architecture is shown below and the aspect under audit is outlined in red.
User deposits are held by the core Prize Pool, Yield Source implementations, and the corresponding tokens.
The PrizePool contract is the primary point of entry for users: deposits and withdrawals occur here. The contract is abstract, and has two subclasses that handle escrowed funds differently: StakePrizePool and YieldSourcePrizePool.
This subclass of the PrizePool ferries deposits into an external Yield Source contract.
The StakePrizePool is a subclass of the PrizePool that simply holds the deposited tokens. No yield is generated, so prizes must be added manually.
The ControlledToken extends the standard ERC20 token (OpenZeppelin flavour) and adds a "token controller" is that able to burn and mint tokens. The controller can also "listen" to token transfers and is called anytime a transfer occurs. The token additionally extends the (at the time) experimental ERC20Permit contract provided by OpenZeppelin.
When a Prize Pool is created an array of Controlled Tokens is passed to it. These tokens serve as receipts for the deposited collateral. The controller for all of the Controlled Tokens for a prize pool must be the prize pool itself.
The Ticket extends the above Controlled Token and adds a special data structure that organizes the token holders into contiguous blocks. Visualizations can be found in this article.
The ATokenYieldSource is an adapter for Aave V2 Lending Pools that implements the IYieldSource.sol interface.
The YearnV2YieldSource is an adapter for Yearn V2 Vaults that implements the IYieldSource.sol interface.
The SushiYieldSource is an adapter for SushiBar that implements the IYieldSource.sol interface.
The IdleYieldSource is an adapter for Idle's Idle Token that implements the IYieldSource.sol interface.
The Badger Yield Source is an adapter for Badger Sett Vaults.
Here are some thing we're guessing may be problematic, so want to point out for efficiency's sake.
These contracts make use of floating point math. Underflow and overflow are always a concern, especially when packing token amounts into uint128. Rounding has also been known to be challenging, as numbers can behave unexpectedly.
As a consequence of the yield source interface, there is a common pattern among yield sources to exchange the tokens to shares then burn the shares.
Notice in this code from the YearnV2YieldSource.sol#L140:
function redeemToken(uint256 amount) external override returns (uint256) { uint256 shares = _tokenToShares(amount); uint256 withdrawnAmount = _withdrawFromVault(amount); _burn(msg.sender, shares); token.safeTransfer(msg.sender, withdrawnAmount); emit RedeemedToken(msg.sender, shares, amount); return withdrawnAmount; }
The user is attempting to redeem say, 10 Dai, but the actual burned number of shares is calculated using the exchange rate. Are there boundary conditions in this math that could be problematic? Will it be impossible to withdraw everything and so dust will accrue? Will this contract ever lock up indefinitely?
We've been careful to prevent the addition of malicious Controlled Tokens after a prize pool has been created, so that users can't be rugged by a manipulated token supply. Are there any other ways that deposits can be compromised?
The token listener pattern used throughout the code could potentially lock tokens and grief users if exposed to the world. We've locked them down as best we can, but are there any exploits that could grief users?
There could easily be things I have missed. This is why we need your keen eyes!