search

code-423n4 / 2022-06-yieldy-findings

0 stars 0 forks source link

Unsafe `transferFrom()` #206

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Migration.sol#L48

Vulnerability details

Impact

Yieldy.transferFrom() returns false on failure instead of reverting. This might lead to moveFundsToUpgradedContract() incorrectly unstaking and restaking tokens, potentially causing user or Migration.sol to lose funds depending on NEW_CONTRACT and OLD_CONTRACT implementations.

Proof of Concept

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Migration.sol#L48-L52

        IYieldy(OLD_YIELDY_TOKEN).transferFrom(
            msg.sender,
            address(this),
            userWalletBalance
        );

No check for boolean return value from transferFrom()

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a check on the return value of transferFrom().

toshiSat commented 2 years ago

sponsor confirmed

0xean commented 2 years ago

This is incorrect, the yieldy transfer call definitely reverts if it fails.

Take a look at the solidity compiler version.

toshiSat commented 2 years ago

I still think it will be good to explicitly check this instead of relying on the compiler version to make it more future proof (even though it most likely won't be affected).