code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

The SwappableYieldSource.sol has a public visibility initialization function that can be front-run, allowing an attacker to incorrectly initialize the contract, if the deployment of this contract does not safely handle initializations via a robust deployment script or a factory contract to prevent front-running.

Impact: Initialization function can be front-run by attackers, allowing them to initialize the contract with malicious values. Also, if initializations are not done atomically with creation, all public/external functions can be accessed before initialization because there are no checks to confirm initializations in those functions.

Proof of Concept

Reference: See similar High-severity Finding 9 of Trail of Bits audit of Advanced Blockchain: https://github.com/trailofbits/publications/blob/master/reviews/AdvancedBlockchain.pdf and Finding 12 from Trail of Bits audit of Hermez Network https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

Ensure atomic creation+deployment with script or factory contract. Add checks to confirm initialization in public/external functions.

PierrickGT commented 3 years ago

This is why we use the freeze function in our deployment: https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/deploy/deploy.ts#L71

0xean commented 3 years ago

The freeze function is not atomic with the deployment and the script does not enforce that that call is made before moving on to further deployments. The script could enforce that the contract has not been initialized which would at least somewhat mitigate the impacts of a potential front run.