Yield-Convex contest
Findings & Analysis Report

2022-03-04

Table of contents

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 code contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the code contest outlined in this document, C4 conducted an analysis of Yield-Convex contest smart contract system written in Solidity. The code contest took place between January 28—January 30 2022.

Wardens

23 Wardens contributed reports to the Yield-Convex contest:

  1. leastwood
  2. kenzo
  3. WatchPug (jtp and ming)
  4. Dravee
  5. sirhashalot
  6. pants
  7. GeekyLumberjack
  8. robee
  9. TomFrenchBlockchain
  10. throttle
  11. Fitraldys
  12. Funen
  13. defsec
  14. rfa
  15. 0x1f8b
  16. cccz
  17. hack3r-0m
  18. hyh
  19. ye0lde
  20. IllIllI
  21. bitbopper
  22. Tomio

This contest was judged by Alex the Entreprenerd.

Final report assembled by liveactionllama and CloudEllie.

Summary

The C4 analysis yielded an aggregated total of 10 unique vulnerabilities and 37 total findings. All of the issues presented here are linked back to their original finding.

Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity, 2 received a risk rating in the category of MEDIUM severity, and 6 received a risk rating in the category of LOW severity.

C4 analysis also identified 9 non-critical recommendations and 18 gas optimizations.

Scope

The code under review can be found within the C4 Yield-Convex contest repository, and is composed of 4 smart contracts and 1 library written in the Solidity programming language and includes 741 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.

Vulnerabilities are divided into three primary risk categories: high, medium, and low.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.

High Risk Findings (2)

[H-01] Malicious Users Can Duplicate Protocol Earned Yield By Transferring wCVX Tokens To Another Account

Submitted by leastwood, also found by kenzo

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.

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.

alcueca (Yield) confirmed and commented:

Confirmed. The fact that rewards can be drained also means that users lose on their expected rewards, so I think that Sev 3 is right.

iamsahu (Yield) resolved

Alex the Entreprenerd (judge) commented:

In systems that track growing rewards, anytime a user balances changes, it’s important to recalculate their balances as to properly distribute pending rewards and to influence the future-rate at which rewards will be distributed (process generally called accruing)

In the case of the ConvexYieldWrapper, the warden has shown that because the wCVX token doesn’t perform a _checkpoint on each transfer, a malicious attacker could repeatedly transfer their tokens in order to reuse the same balance in multiple accounts, effectively sybil attacking the protocol.

The fix seems to be straightforward, however the impact of the finding breaks the accounting of the protocol, as such I believe High Severity to be appropraite

Alex the Entreprenerd (judge) commented:

The sponsor has mitigated in a subsequent PR by overriding the _transfer function

[H-02] Malicious Users Can Transfer Vault Collateral To Other Accounts To Extract Additional Yield From The Protocol

Submitted by leastwood

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.

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.

iamsahu (Yield) confirmed

Alex the Entreprenerd (judge) commented:

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

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 (Yield) commented:

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.

Medium Risk Findings (2)

[M-01] Oracle data feed is insufficiently validated

Submitted by throttle, also found by 0x1f8b, cccz, defsec, hack3r-0m, hyh, kenzo, leastwood, sirhashalot, TomFrenchBlockchain, WatchPug, and ye0lde

Price can be stale and can lead to wrong quoteAmount return value

Proof of Concept

Oracle data feed is insufficiently validated. There is no check for stale price and round completeness. Price can be stale and can lead to wrong quoteAmount return value

function _peek(
    bytes6 base,
    bytes6 quote,
    uint256 baseAmount
) private view returns (uint256 quoteAmount, uint256 updateTime) {
    ...

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

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

    ...
}

Validate data feed

function _peek(
    bytes6 base,
    bytes6 quote,
    uint256 baseAmount
) private view returns (uint256 quoteAmount, uint256 updateTime) {
    ...
    (uint80 roundID, int256 daiPrice, , uint256 timestamp, uint80 answeredInRound) = DAI.latestRoundData();
    require(daiPrice > 0, "ChainLink: DAI price <= 0");
    require(answeredInRound >= roundID, "ChainLink: Stale price");
    require(timestamp > 0, "ChainLink: Round not complete");

    (roundID, int256 usdcPrice, , timestamp, answeredInRound) = USDC.latestRoundData();
    require(usdcPrice > 0, "ChainLink: USDC price <= 0");
    require(answeredInRound >= roundID, "ChainLink: Stale USDC price");
    require(timestamp > 0, "ChainLink: USDC round not complete");

    (roundID, int256 usdtPrice, , timestamp, answeredInRound) = USDT.latestRoundData();
    require(usdtPrice > 0, "ChainLink: USDT price <= 0");
    require(answeredInRound >= roundID, "ChainLink: Stale USDT price");
    require(timestamp > 0, "ChainLink: USDT round not complete");

    ...
}

iamsahu (Yield) confirmed and resolved

Alex the Entreprenerd (judge) commented:

When using Chainlink Price feeds it is important to ensure the price feed data was updated recently. While getting started with chainlink requires just one line of code, it is best to add additional checks for in production environments.

I believe the finding to be valid and Medium severity to be appropriate.

The sponsor has mitigated in a subsequent PR.

[M-02] Rewards distribution can be disrupted by a early user

Submitted by WatchPug

https://github.com/code-423n4/2022-01-yield/blob/e946f40239b33812e54fafc700eb2298df1a2579/contracts/ConvexStakingWrapper.sol#L206-L224

function _calcRewardIntegral(
    uint256 _index,
    address[2] memory _accounts,
    uint256[2] memory _balances,
    uint256 _supply,
    bool _isClaim
) internal {
    RewardType storage reward = rewards[_index];

    uint256 rewardIntegral = reward.reward_integral;
    uint256 rewardRemaining = reward.reward_remaining;

    //get difference in balance and remaining rewards
    //getReward is unguarded so we use reward_remaining to keep track of how much was actually claimed
    uint256 bal = IERC20(reward.reward_token).balanceOf(address(this));
    if (_supply > 0 && (bal - rewardRemaining) > 0) {
        rewardIntegral = uint128(rewardIntegral) + uint128(((bal - rewardRemaining) * 1e20) / _supply);
        reward.reward_integral = uint128(rewardIntegral);
    }

reward.reward_integral is uint128, if a early user mint (wrap) just 1 Wei of convexToken, and make _supply == 1, and then tranferring 5e18 of reward_token to the contract.

As a result, reward.reward_integral can exceed type(uint128).max and overflow, causing the rewards distribution to be disrupted.

Recommendation

Consider wrap a certain amount of initial totalSupply, e.g. 1e8, and never burn it. And consider using uint256 instead of uint128 for reward.reward_integral. Also, consdier lower 1e20 down to 1e12.

alcueca (Yield) confirmed but disagreed with severity and commented:

Assets are not a direct risk if it is the first user disrupting the contract. We would need to redeploy better code, but that’s it. I suggest this is reported as medium, as merits for a DoS attack.

As suggested elsewhere, the right solution if uint128 is to be used in storage, is to cast up to uint256 for calculations, and then back to uint128 to store again.

iamsahu (Yield) resolved

Alex the Entreprenerd (judge) changed severity from High to Medium and commented:

The warden identified a way an early depositor can grief the system, I believe the finding to be valid, and because:

  • It would conditionally disrupt the system
  • It would “brick the contract” at the loss of the griefer
  • No additional funds would be at risk

I believe medium severity to be appropriate

Low Risk Findings (6)

Non-Critical Findings (9)

Gas Optimizations (18)

Disclosures

C4 is an open organization governed by participants in the community.

C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.