Yield-Convex contest - leastwood's results

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.

During wrap() and unwrap() actions, _checkpoint() is used to update the rewards for the from_ and to_ accounts. However, the reference contract implements a _beforeTokenTransfer() function which has been removed from Yield Protocol's custom implementation.

As a result, it is possible to transfer wCVX tokens to another account after an initial checkpoint has been made. By manually calling user_checkpoint() on the new account, this user is able to update its deposited balance of the new account while the sender's balance is not updated. This can be repeated to effectively replicate a user's deposited balance over any number of accounts. To claim yield generated by the protocol, the user must only make sure that the account calling getReward() holds the tokens for the duration of the call.

Proof of Concept

The exploit can be outlined through the following steps:

• Alice receives 100 wCVX tokens from the protocol after wrapping their convex tokens.
• 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 transfers her tokens to her friend Bob who then manually calls user_checkpoint() to update his balance.
• Now from the perspective of the protocol, both Alice and Bob have 100 wCVX tokens as calculated by the _getDepositedBalance() function.
• If either Alice or Bob wants to claim rewards, all they need to do is make sure the 100 wCVX tokens are in their account upon calling getReward(). Afterwards, the tokens can be transferred out.

Tools Used

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

Recommended Mitigation Steps

Consider implementing the _beforeTokenTransfer() function as shown in the reference contract. However, it is important to ensure the wrapper contract and collateral vaults are excluded from the checkpointing so they are not considered in the rewards calculations.

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.

Handle

leastwood

Vulnerability details

Impact

latestRoundData is missing additional validation to ensure that the round is complete and has returned a valid/expected price. This is documented here.

Proof of Concept

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

(, int256 daiPrice, , , ) = DAI.latestRoundData();
(, int256 usdcPrice, , , ) = USDC.latestRoundData();
(, int256 usdtPrice, , , ) = USDT.latestRoundData();

require(
    daiPrice > 0 && usdcPrice > 0 && usdtPrice > 0,
    "Chainlink pricefeed reporting 0"
);

Tools Used

Manual code review. Chainlink best practices.

Recommended Mitigation Steps

Consider validating the output of latestRoundData to match the following code snippet:

(
    uint80 roundID,
    int256 daiPrice,
    ,
    uint256 updateTime,
    uint80 answeredInRound
) = DAI.latestRoundData();
require(
    answeredInRound >= roundID,
    "Yield: Chainlink Price Stale"
);
require(daiPrice > 0, "Yield: Chainlink Malfunction");
require(updateTime != 0, "Yield: Incomplete round");

The same recommendation can be applied to the USDC.latestRoundData() and USDT.latestRoundData() calls.