code423n4 commented 2 years ago

Handle

leastwood

Vulnerability details

Impact

ConvexYieldWrapper.sol is a wrapper contract for staking convex tokens on the user's behalf, allowing them to earn rewards on their deposit. Users will interact with the Ladle.sol contract's batch() function which:

Approves Ladle to move the tokens.
Transfers the tokens to ConvexYieldWrapper.sol.
Wraps/stakes these tokens.
Updates accounting and produces debt tokens within Ladle.sol.

_getDepositedBalance() takes into consideration the user's total collateral stored in all of their owned vaults. However, as a vault owner, you are allowed to give the vault to another user, move collateral between vaults and add/remove collateral. Therefore, it is possible to manipulate the result of this function by checkpointing one user's balance at a given time, transferring ownership to another user and then create a new checkpoint with this user.

As a result, a user is able to generate protocol yield multiple times over on a single collateral amount. This can be abused to effectively extract all protocol yield.

Proof of Concept

Consider the following exploit scenario:

Alice owns a vault which has 100 tokens worth of collateral.
At that point in time, _getDepositedBalance() returns 100 as its result. A checkpoint has also been made on this balance, giving Alice claim to her fair share of the rewards.
Alice then calls Ladle.give(), transferring the ownership of the vault to Bob and calls ConvexYieldWrapper.addVault().
Bob is able to call user_checkpoint() and effectively update their checkpointed balance.
At this point in time, both Alice and Bob have claim to any yield generated by the protocol, however, there is only one vault instance that holds the underlying collateral.

https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexYieldWrapper.sol#L100-L120

function _getDepositedBalance(address account_) internal view override returns (uint256) {
    if (account_ == address(0) || account_ == collateralVault) {
        return 0;
    }

    bytes12[] memory userVault = vaults[account_];

    //add up all balances of all vaults registered in the wrapper and owned by the account
    uint256 collateral;
    DataTypes.Balances memory balance;
    uint256 userVaultLength = userVault.length;
    for (uint256 i = 0; i < userVaultLength; i++) {
        if (cauldron.vaults(userVault[i]).owner == account_) {
            balance = cauldron.balances(userVault[i]);
            collateral = collateral + balance.ink;
        }
    }

    //add to balance of this token
    return _balanceOf[account_] + collateral;
}

Tools Used

Manual code review. Discussion/confirmation with the Yield Protocol team.

Recommended Mitigation Steps

Ensure that any change to a vault will correctly checkpoint the previous and new vault owner. The affected actions include but are not limited to; transferring ownership of a vault to a new account, transferring collateral to another vault and adding/removing collateral to/from a vault.

GalloDaSballo commented 2 years ago

The warden identified a way to sidestep the accounting in the ConvexYieldWrapper.

Because ConvexYieldWrapper takes lazy accounting, transferring vaults at the Ladle level allows to effectively register the same vault under multiple accounts, which ultimately allow to steal more yield than expected.

While the loss of yield can be classified as a medium severity, the fact that the warden was able to break the accounting invariants of the ConvexYieldWrapper leads me to raise the severity to high

GalloDaSballo commented 2 years ago

Ultimately mitigation will require to _checkpoint also when vault operations happen (especially transfer), this may require a rethinking at the Ladle level as the reason why the warden was able to sidestep the checkpoint is because the Ladle doesn't notify the Wrapper of any vault transfers

alcueca commented 2 years ago

Yes, that's right. To fix this issue we will deploy a separate Ladle to deal specifically with convex tokens. The fix will probably involve removing stir and give instead of notifying the wrapper, but we'll see.

code-423n4 / 2022-01-yield-findings