Yeti Finance contest - kenzo's results

Handle

kenzo

Vulnerability details

StabilityPool's receiveCollateral should only be called by ActivePool, but that check is missing. Anybody can call it and update StabilityPool's total collateral variable.

Impact

Wrong amounts of total collateral in StabilityPool (totalColl). As far as I can see, this value is only used in view functions.

Proof of Concept

receiveCollateral: (notice comment and lack of caller check) (Code ref)

    // Should be called by ActivePool
    // __after__ collateral is transferred to this contract from Active Pool
    function receiveCollateral(address[] memory _tokens, uint256[] memory _amounts)
        external
        override
    {
        totalColl.amounts = _leftSumColls(totalColl, _tokens, _amounts);
        emit StabilityPoolBalancesUpdated(_tokens, _amounts);
    }

The rest of the pools do check the caller on this function.

Recommended Mitigation Steps

Add _requireCallerIsActivePool() to the function.

Handle

kenzo

Vulnerability details

WJLP doesn't update the inner accounting (for JOE rewards) when unwrapping user's tokens. The user will continue to receive rewards, on the expanse of users who haven't claimed their rewards yet.

Impact

Loss of yield for users.

Proof of Concept

The unwrap function just withdraws JLP from MasterChefJoe, burns the user's WJLP, and sends the JLP back to the user. It does not update the inner accounting (userInfo). (Code ref)

    function unwrapFor(address _to, uint _amount) external override {
        _requireCallerIsAPorSP();
        _MasterChefJoe.withdraw(_poolPid, _amount);
        // msg.sender is either Active Pool or Stability Pool
        // each one has the ability to unwrap and burn WAssets they own and
        // send them to someone else
        _burn(msg.sender, _amount);
        JLP.transfer(_to, _amount);
    }

Recommended Mitigation Steps

Need to keep userInfo updated. Have to take into consideration the fact that user can choose to set the reward claiming address to be a different account than the one that holds the WJLP.

Handle

kenzo

Vulnerability details

After updating user's rewards in _userUpdate, if the user has not claimed them, and _userUpdate is called again (eg. on another wrap), the user's unclaimed rewards will lose the previous unclaimed due to wrong calculation.

Impact

Loss of yield for user.

Proof of Concept

When updating the user's unclaimedJoeReward, the function doesn't save it's previous value. (Code ref)

        if (user.amount > 0) {
            user.unclaimedJOEReward = user.amount.mul(accJoePerShare).div(1e12).sub(user.rewardDebt);
        }
        if (_isDeposit) {
            user.amount = user.amount.add(_amount);
        } else {
            user.amount = user.amount.sub(_amount);
        }
        // update for JOE rewards that are already accounted for in user.unclaimedJOEReward
        user.rewardDebt = user.amount.mul(accJoePerShare).div(1e12);

So for example, rewards can be lost in the following scenario. We'll mark "acc1" for the value of "accJoePerShare" at step 1.

1. User Zebulun wraps 100 tokens. After _userUpdate is called: unclaimedJOEReward = 0, rewardDebt = 100*acc1.
2. Zebulun wraps 50 tokens: unclaimedJOEReward = 100acc2 - 100acc1, rewardDebt = 150 * acc2.
3. Zebulun wraps 1 token: unclaimedJOEReward = 150acc3 - 150acc2, rewardDebt = 151*acc3 So in the last step, Zebulun's rewards only take into account the change in accJoePerShare in steps 2-3, and lost the unclaimed rewards from steps 1-2.

Recommended Mitigation Steps

Change the unclaimed rewards calculation to:

user.unclaimedJOEReward = user.unclaimedJOEReward.add(user.amount.mul(accJoePerShare).div(1e12).sub(user.rewardDebt));

Handle

kenzo

Vulnerability details

WJLP's wrap takes _from and _to as parameter, and doesn't check that msg.sender=_from. This means that anybody can claim to himself tokens that a user approved for WJLP.

Impact

Loss of user funds.

Proof of Concept

The problem is in the wrap function. (Code ref)

function wrap(uint _amount, address _from, address _to, address _rewardOwner) external override {
        JLP.transferFrom(_from, address(this), _amount);
        ...
        _mint(_to, _amount);
}

No check is being done on from\to parameters. After a user has .approved WJLP, a malicious user can call wrap(_amount, fromLegitimateUser, toMaliciousUser, toMaliciousUser) and gain the legitimate user's tokens.

Recommended Mitigation Steps

Probably remove _from from the parameters, and wrap only msg.sender's tokens.

Handle

kenzo

Vulnerability details

WJLP can only be unwrapped from the Active Pool or Stability Pool. A user who decided to wrap his JLP, but not use all of them in a trove, Wouldn't be able to just unwrap them.

Impact

Impaired functionality for users. Would have to incur fees for simple unwrapping.

Proof of Concept

The unwrap functionality is only available from unwrapFor function, and that function is only callable from AP or SP. (Code ref)

function unwrapFor(address _to, uint _amount) external override {
        _requireCallerIsAPorSP();

Recommended Mitigation Steps

Allow anybody to call the function. As it will burn the holder's WJLP, a user could only unwrap tokens that are not in use.

Handle

kenzo

Vulnerability details

When ActivePool sends collateral which is a wrapped asset, it first unwraps the asset, and only after that updates the rewards. This should be done in opposite order. As a comment in WJLP's unwrapFor rightfully mentions - "Prior to this being called, the user whose assets we are burning should have their rewards updated".

Impact

Lost yield for user.

Proof of Concept

In ActivePool's sendCollateralsUnwrap (which is used throughout the protocol), it firsts unwraps the asset, and only afterwards calls claimRewardFor which will update the rewards: (Code ref)

IWAsset(_tokens[i]).unwrapFor(_to, _amounts[i]);
if (_collectRewards) {
        IWAsset(_tokens[i]).claimRewardFor(_to);
}

claimRewardFor will end up calling _userUpdate: (Code ref)

    function _userUpdate(address _user, uint256 _amount, bool _isDeposit) private returns (uint pendingJoeSent) {
        uint256 accJoePerShare = _MasterChefJoe.poolInfo(_poolPid).accJoePerShare;
        UserInfo storage user = userInfo[_user];
        if (user.amount > 0) {
            user.unclaimedJOEReward = user.amount.mul(accJoePerShare).div(1e12).sub(user.rewardDebt);
        }
        if (_isDeposit) {
            user.amount = user.amount.add(_amount);
        } else {
            user.amount = user.amount.sub(_amount);
        }
        user.rewardDebt = user.amount.mul(accJoePerShare).div(1e12);
    }

Now, as ActivePool has already called unwrapFor and has burnt the user's tokens, and let's assume they all were used as collateral, it means user.amount=0*, and the user's unclaimedJOEReward won't get updated to reflect the rewards from the last user update. This is why, indeed as the comment in unwrapFor says, user's reward should be updated prior to that.

*Note: at the moment unwrapFor doesn't updates the user's user.amount, but as I detailed in another issue, that's a bug, as that means the user will continue accruing rewards even after his JLP were removed from the protocol.

Recommended Mitigation Steps

Change the order of operations in sendCollateralsUnwrap to first send the updated rewards and then unwrap the asset. You can also consider adding to the beginning of unwrapFor a call to _userUpdate(_to, 0, true) to make sure the rewards are updated before unwrapping. Note: as user can choose to have JOE rewards accrue to a different address than the address that uses WJLP as collateral, you'll have to make sure you update the current accounts. I'll detail this in another issue.