Yield source does not correctly calculate share conversions

[code423n4]

Lines of code

https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L352-L374

Vulnerability details

The aTokens’ value is pegged to the value of the corresponding supplied asset at a 1:1 ratio and can be safely stored, transferred or traded. All yield collected by the aTokens' reserves are distributed to aToken holders directly by continuously increasing their wallet balance.

https://docs.aave.com/developers/tokens/atoken

Impact

Incorrect share conversions lead to incorrect pricig of assets and loss of principal. aTokens are rebasing tokens, which means that holders of the token have their balanceof() increase over time, but each token is still redeemable for exactly one underlying asset. Any formula that does not return one out for one in is incorrect.

Proof of Concept

File: contracts/AaveV3YieldSource.sol   #X

352     /**
353      * @notice Calculates the number of shares that should be minted or burnt when a user deposit or withdraw.
354      * @param _tokens Amount of asset tokens
355      * @return Number of shares.
356      */
357     function _tokenToShares(uint256 _tokens) internal view returns (uint256) {
358       uint256 _supply = totalSupply();
359   
360       // shares = (tokens * totalShares) / yieldSourceATokenTotalSupply
361       return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this)));
362     }
363   
364     /**
365      * @notice Calculates the number of asset tokens a user has in the yield source.
366      * @param _shares Amount of shares
367      * @return Number of asset tokens.
368      */
369     function _sharesToToken(uint256 _shares) internal view returns (uint256) {
370       uint256 _supply = totalSupply();
371   
372       // tokens = (shares * yieldSourceATokenTotalSupply) / totalShares
373       return _supply == 0 ? _shares : _shares.mul(aToken.balanceOf(address(this))).div(_supply);
374     }

https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L352-L374

The above code is used for both supplyTokenTo() and redeemToken() and does not return one for one. Consider the following chain of events:

1. There are no deposits yet
2. Alice deposits one wBTC, getting back a AaveV3YieldSource share, while the yield source gets the aToken
3. Some time later a total of one extra wBTC worth of aToken is generated as yield and is in the balanceOf(this)
4. Alice attempts to withdraw her one share but gets zero wBTC, because (tokens{1} * totalSupply(){1}) / aToken.balanceOf(this){2} is zero

Tools Used

Code inspection

Recommended Mitigation Steps

There does not need to be a conversion function - one share must always equal one token

[PierrickGT]

This is a plausible but unlikely scenario since it assumes that only Alice has deposited into the yield source and that the interest rate is so high that 1 more wBTC is now in the yield source. Also, the calculation is wrong since 1 wBTC would represent 1 ether or 1000000000000000000 wei. So the actual calculation would return (1000000000000000000 * 1000000000000000000) / 2000000000000000000 = 500000000000000000 wei or 0.5 wBTC.

We also periodically capture the interest accumulated in the yield source by calling the PrizeFlush contract: https://github.com/pooltogether/v4-periphery/blob/8cc717af440aa23e41e0f24d699647320efd7b04/contracts/PrizeFlush.sol#L105 The capture of the interest happens here: https://github.com/pooltogether/v4-core/blob/e6f2d9ddb303cc4fb1a64582b12ba3e9df85e21c/contracts/prize-strategy/PrizeSplitStrategy.sol#L51

For these reasons, I've acknowledged the issue but we won't change how shares are calculated.

[gititGoro]

Downgraded severity as this is more of a value leakage situation, especially given the unlikely edge case of an ERC20 token that sets decimals to 0 and uses low base values.