Yieldy contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L89

Vulnerability details

Impact

Deleted contract address index are not moved on removeAddress function at BatchRequests.sol#L89.

All deleted index will become 0 so contract address will start having 0 address values. This becomes problem at canBatchContracts which will now fail since IStaking(contracts[i]).canBatchTransactions() will fail on 0 address

Proof of Concept

1. Assume 10 address are present in contracts

2. Admin decides to remove address X which is present at index contracts[2]

3. Admin calls removeAddress for same which makes contracts[2] = address(0)

4. User calls canBatchContracts which fails since IStaking(contracts[i]).canBatchTransactions() will fail on 0 address

Recommended Mitigation Steps

Ideally deleted index should be replaced by last index and a pop should be done

Lines of code

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

Vulnerability details

Impact

It is not checked whether call to transferFrom has succeded or not in moveFundsToUpgradedContract function. This becomes a problem and could lead to contract losing funds

Proof of Concept

1. User A calls moveFundsToUpgradedContract
2. Unfortunately due to some reasons, call to IYieldy(OLD_YIELDY_TOKEN).transferFrom(...) fails so no user fund are transferred to contract
3. IStaking(NEW_CONTRACT).stake(userWalletBalance, msg.sender); will stake userWalletBalance for User A even though User A paid nothing

Recommended Mitigation Steps

Check return value of transferFrom. If it is false then revert

bool success = IYieldy(OLD_YIELDY_TOKEN).transferFrom(
            msg.sender,
            address(this),
            userWalletBalance
        );
require(success, "transfer failed");

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Yieldy.sol#L91

Vulnerability details

Impact

It was observed that if updatedTotalSupply > MAX_SUPPLY then updatedTotalSupply becomes MAX_SUPPLY. This means _profit amount is not fully used. But _storeRebase function is still called with _profit amount

This becomes a problem since _storeRebase function caluclates rebasePercent using this incorrect _profit amount

Proof of Concept

1. REBASE_ROLE calls rebase function with say profit 10. Assume currentTotalSupply is 90

2. updatedTotalSupply is calculated as updatedTotalSupply = currentTotalSupply + _profit. Thus updatedTotalSupply becomes 90+10=100

3. Assume MAX_SUPPLY is 91. Since updatedTotalSupply>MAX_SUPPLY so updatedTotalSupply is updated to be 91

4. Now _storeRebase function is called with updatedTotalSupply (91), _profit(10)

5. This is incorrect since 10 amount from _profit is not utilized and only 1 amount is utilized. This becomes a problem in rebasePercent calculation where it is calculated on full 10 amount instead of 1

Recommended Mitigation Steps

Use below:

if (updatedTotalSupply > MAX_SUPPLY) {
_profit=_profit - (updatedTotalSupply-MAX_SUPPLY);
                updatedTotalSupply = MAX_SUPPLY;
				
            }

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/LiquidityReserve.sol#L161

Vulnerability details

Impact

Whale who provided most liquidity to the contract can simply use removeLiquidity function and can remove all of his liquidity. This can leave the residual liquidity to be less than MINIMUM_LIQUIDITY which is incorrect

Proof of Concept

1. Whale A provided initial liquidity plus more liquidity using enableLiquidityReserve and addLiquidity function

2. There are other small liquidity providers as well

3. Now Whale A decides to remove all the liquidity provided

4. This means after liquidity removal the balance liquidity will even drop below MINIMUM_LIQUIDITY which is incorrect

Recommended Mitigation Steps

Add below check

require(
            IERC20Upgradeable(stakingToken).balanceOf(address(this)) - MINIMUM_LIQUIDITY >=
                amountToWithdraw,
            "Not enough funds"
        );

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Staking.sol#L319

Vulnerability details

Impact

_requestWithdrawalFromTokemak function :: Instead of sending amountToRequest for requestWithdrawal, contract is asking _amount for requestWithdrawal. This becomes a problem when balance < _amount and only balance could be withdrawn

Proof of Concept

1. In _requestWithdrawalFromTokemak function, amountToRequest is calculated as

// the only way balance < _amount is when using unstakeAllFromTokemak
        uint256 amountToRequest = balance < _amount ? balance : _amount;

2. Now assuming balance < _amount then amountToRequest becomes balance

3. But tokePoolContract.requestWithdrawal is called over _amount instead of amountToRequest which means withdrawal is requested over an extra amount

Recommended Mitigation Steps

Modify Staking.sol#L326 to

if (amountToRequest > 0) tokePoolContract.requestWithdrawal(amountToRequest);

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L19

Function sendWithdrawalRequests: No need to check IStaking(contracts[i]).canBatchTransactions() as this is already done in IStaking(contracts[i]).sendWithdrawalRequests();

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/LiquidityReserve.sol#L161

Function removeLiquidity: require(isReserveEnabled, "Not enabled yet"); should be added otherwise there is no liquidity to remove

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Staking.sol#L88

Function initialize: L88-L91 is complete duplicate of L84-L87 . Remove either one of them