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

0 stars 0 forks source link

Incorrect address check in transferERC20 can allow rugging #43

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

SwappableYieldSource.sol has a transferERC20() function callable only by the owner or asset manager to transfer out ERC20 tokens other than the yield source's tokens held by this contract. This is similar to the functions in ATokenYieldSource and IdleYieldSource.

In ATokenYieldSource and IdleYieldSource, the token exclusion check is made on aToken and idleToken respectively because they are specific to those yield contracts.

However, in this generic SwappableYieldSource contract which points to the chosen yield source, there is not one permanent yield source token. So the exclusion check has to be performed on the token of the current yield source. But the check implemented checks against the address of the yieldSource contract instead of its token. This will pass even for the yieldSource token and will allow owner or asset manager to transfer out at any time all the yieldSource tokens to any recipient address of choice i.e. rugging.

For e.g. if SwappableYieldSource yieldSource points to ATokenYieldSource, the check will allow aToken to be transferred out because it checks the ERC20 aToken address against ATokenYieldSource address which will pass and allow the transfer out.

Proof of Concept

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L324

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L317-L328

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/yield-source/ATokenYieldSource.sol#L225

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/yield-source/IdleYieldSource.sol#L142

Tools Used

Manual Analysis

Recommended Mitigation Steps

Check against yield source token address instead of yield source address.

PierrickGT commented 3 years ago

This exploit is not possible. The swappable yield source don't old any funds, this is the role of the yield source that either mint ERC20 token shares to the swappable yield source or keep track of the balance by using a mapping. That's why we only check that transferERC20 can't move ERC20 token shares that would represent the swappable yield source shares in the yield source.

0xean commented 3 years ago

I agree that there is a potential for a rug pull here, it would have to be in combination with #14 but the potential exists. I am marking as a duplicate as #14 as the issues are related and the check here does little to avoid tokens being transferred after the yieldSource has been changed.

PierrickGT commented 3 years ago

Closing this issue that has been marked as a duplicate.