Yieldy contest - hake's results

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().

Lines of code

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

Vulnerability details

Impact

stake() will revert for tokens that charge a fee on transfer.

Proof of Concept

Note: POC below assumes tokePoolContract.deposit(_amount) transfers part of Staking.sol balance to tokePoolContract.

stake() uses the _amount as a reference for _depositToTokemak() and assumes the amount received is equal to _amount. However, with fee on transfer tokens the amount received by Staking.sol will be inferior to _amount due to the fee charged on transfer by the token.

Tools Used

Manual Review

Recommended Mitigation Steps

Check the difference between the before and after balance to ensure amount used to deposit is equal to the amount Staking.sol has received.

QA Report

[L-01] Approve has been deprecated

I recommend using safeIncreaseAllowance() from OpenZeppelin.

OpenZeppelin Documentation

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

[N-01] Typo

@ notice remove address to contracts array

Change to: remove address from contracts array https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L86

[N-02] Change naming to reflect functionality

using the word enable in enableLiquidityReserve() implies it can be disabled. I suggest changing the word "enable" to "init" stemming from initialise. https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L57