Yeti Finance contest - csanuragjain's results

Handle

csanuragjain

Vulnerability details

Impact

Contract instability and financial loss. This will happen if one of the allowed contract calls sendCollaterals with non whitelisted token (may happen with user input on allowed contract)

Proof of Concept

1. Navigate to contract at https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/ActivePool.sol

2. Assume sendCollaterals function is called by one of allowed contract with a non whitelisted token and amount as 1

function sendCollaterals(address _to, address[] memory _tokens, uint[] memory _amounts) external override returns (bool) {
        _requireCallerIsBOorTroveMorTMLorSP();
        require(_tokens.length == _amounts.length);
        for (uint i = 0; i < _tokens.length; i++) {
            _sendCollateral(_to, _tokens[i], _amounts[i]); // reverts if send fails
        }

        if (_needsUpdateCollateral(_to)) {
            ICollateralReceiver(_to).receiveCollateral(_tokens, _amounts);
        }
        
        return true;
    }

3. This calls _sendCollateral with our non whitelisted token and amount as 1

function _sendCollateral(address _to, address _collateral, uint _amount) internal returns (bool) {
        uint index = whitelist.getIndex(_collateral);
        poolColl.amounts[index] = poolColl.amounts[index].sub(_amount);
        bool sent = IERC20(_collateral).transfer(_to, _amount);
        require(sent);

        emit ActivePoolBalanceUpdated(_collateral, _amount);
        emit CollateralSent(_collateral, _to, _amount);
    }

4. whitelist.getIndex(_collateral); will return 0 as our collateral is not whitelisted and will not be present in whitelist.getIndex(_collateral);. This means index will point to whitelisted collateral at index 0

5. poolColl.amounts[index] will get updated for whitelisted collateral at index 0 even though this collateral was never meant to be updated

poolColl.amounts[index] = poolColl.amounts[index].sub(_amount);

6. Finally our non supported token gets transferred to recipient and since _needsUpdateCollateral is true so recipient poolColl.amounts gets increased even though recipient never received the whitelisted collateral

7. Finally sender pool amount will be reduced even though it has the whitelisted collateral and recipient pool amount will be increased even though it does not have whitelisted collateral

Recommended Mitigation Steps

Add a check to see if collateral to be transferred is whitelisted

Handle

csanuragjain

Vulnerability details

Impact

Duplicate collaterals can be added which makes getValidCollateral return duplicate items. This impacts all function which uses getValidCollateral function like _getPendingCollRewards, which will now calculate the pending reward twice for the duplicate item. This only impact collateral at index 0

Proof of Concept

1. Navigate to contract at https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/Dependencies/Whitelist.sol

2. Assume addCollateral function is called by owner with collateral address say 0xabc...

3. This collateral gets pushed to the system via validCollateral.push(_collateral);

4. Admin calls addCollateral function again and by mistake passes collateral address as 0xabc... again. Below if condition is bypassed since validCollateral[0] != _collateral is false (This collateral is already at index 0)

if (validCollateral.length != 0 && validCollateral[0] != _collateral) {
            require(collateralParams[_collateral].index == 0, "collateral already exists");
        }

5. This means now collateral 0xabc... is present at both index 0 and 1 of validCollateral. Also the previous collateralParams[0xabc...] for index 0 will be overwritten (say if admin disabled this collateral initially, this will get reenabled once duplicate entry is added)

6. This will also impact all function calling getValidCollateral as now they will get duplicate collateral and thus will perform operation twice on same token (_getPendingCollRewards, which will now calculate the pending reward twice for the duplicate item.)

Recommended Mitigation Steps

Return error if owner adds a collateral which is already present at index 0

Handle

csanuragjain

Vulnerability details

Impact

Monetary loss for user

Proof of Concept

1. Navigate to contract at https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol

2. Let us see _sendJoeReward function

function _sendJoeReward(address _rewardOwner, address _to) internal {
        // harvests all JOE that the WJLP contract is owed
        _MasterChefJoe.withdraw(_poolPid, 0);

        // updates user.unclaimedJOEReward with latest data from TJ
        _userUpdate(_rewardOwner, 0, true);

        uint joeToSend = userInfo[_rewardOwner].unclaimedJOEReward;
        userInfo[_rewardOwner].unclaimedJOEReward = 0;
        _safeJoeTransfer(_to, joeToSend);
    }

3. Lets say user reward are calculated to be 100 so _safeJoeTransfer is called with joeToSend as 100. Also user remaining reward becomes 0

4. Let us see _safeJoeTransfer function

function _safeJoeTransfer(address _to, uint256 _amount) internal {
        uint256 joeBal = JOE.balanceOf(address(this));
        if (_amount > joeBal) {
            JOE.transfer(_to, joeBal);
        } else {
            JOE.transfer(_to, _amount);
        }
    }

5. If the reward balance left in this contract is 90 then _safeJoeTransfer will pass if condition and contract will transfer 90 amount. Thus user incur a loss of 100-90=10 amount (his remaining reward are already set to 0)

Recommended Mitigation Steps

If the reward balance is lower than user balance then contract must transfer reward balance in contract and make remaining user reward balance as ( user reward balance - contract reward balance )