Asymmetry Finance afETH Invitational - MiloTruck's results

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L31-L33

Vulnerability details

Bug Description

In VotiumStrategy.sol, the price of vAfEth is determined by the price() function:

VotiumStrategy.sol#L31-L33

    function price() external view override returns (uint256) {
        return (cvxPerVotium() * ethPerCvx(false)) / 1e18;
    }

As seen from above, it calls ethPerCVX() with false to determine the price of CVX in ETH. ethPerCVX() relies on a Chainlink oracle to fetch the CVX / ETH price:

VotiumStrategyCore.sol#L156-L183

    function ethPerCvx(bool _validate) public view returns (uint256) {
        ChainlinkResponse memory cl;
        try chainlinkCvxEthFeed.latestRoundData() returns (
            uint80 roundId,
            int256 answer,
            uint256 /* startedAt */,
            uint256 updatedAt,
            uint80 /* answeredInRound */
        ) {
            cl.success = true;
            cl.roundId = roundId;
            cl.answer = answer;
            cl.updatedAt = updatedAt;
        } catch {
            cl.success = false;
        }
        // verify chainlink response
        if (
            (!_validate ||
                (cl.success == true &&
                    cl.roundId != 0 &&
                    cl.answer >= 0 &&
                    cl.updatedAt != 0 &&
                    cl.updatedAt <= block.timestamp &&
                    block.timestamp - cl.updatedAt <= 25 hours))
        ) {
            return uint256(cl.answer);
        } else {

If ethPerCvx() is called with _validate = false, no validation will be performed on Chainlink's price feed. This means that ethPerCVX() could return an invalid price if:

1. The price feed is stale. This would cause ethPerCvx() to return an outdated price that might deviate far from the actual value of CVX.
2. latestRoundData() reverts. Since latestRoundData() is wrapped in a try-catch, cl.answer will not be updated, thus ethPerCvx() would return 0.

Note that scenario 2 could occur if Chainlink's multisigs decide to block access to the CVX / ETH price feed, as described here:

While currently there’s no whitelisting mechanism to allow or disallow contracts from reading prices, powerful multisigs can tighten these access controls. In other words, the multisigs can immediately block access to price feeds at will. Therefore, to prevent denial of service scenarios, it is recommended to query ChainLink price feeds using a defensive approach with Solidity’s try/catch structure.

This becomes problematic as the price of vAfEth is used when calculating the price of AfEth in its price() function:

AfEth.sol#L138-L140

        uint256 vEthValueInEth = (vEthStrategy.price() *
            vEthStrategy.balanceOf(address(this))) / 1e18;
        return ((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply();

Where:

• safEthValueInEth is the amount of safETH locked in ETH.
• vEthValueInEth is the amount of vAfEth locked in ETH.

As seen from above, the price of AfETH is determined as the sum of safETH and vAfEth value divided by totalSupply(), which uses vAfEth's price (vEthStrategy.price()) in its calculation.

AfEth's price is used to determine how much AfEth is minted to the user when deposit() is called:

AfEth.sol#L162-L166

        totalValue +=
            (sMinted * ISafEth(SAF_ETH_ADDRESS).approxPrice(true)) +
            (vMinted * vStrategy.price());
        if (totalValue == 0) revert FailedToDeposit();
        uint256 amountToMint = totalValue / priceBeforeDeposit;

Where:

• priceBeforeDeposit is AfEth's price, determined by calling its price() function.

However, since no validation is performed for Chainlink's price feed when vStrategy.price() is called, users can still call deposit() even when ethPerCvx() returns an incorrect price.

Should this occur, depositors will receive less AfEth for their deposit than expected, since both vStrategy.price() and priceBeforeDeposit will be smaller than usual. In extreme scenarios, if ethPerCvx() returns 0, vMinted * vStrategy.price() will also be 0, therefore the depositor will only receive AfEth for the amount used to buy safETH.

Impact

Since Chainlink's CVX / ETH price feed is not validated when users call deposit(), users that call deposit() while the price feed is stale/blocked will receive less afEth than expected, resulting in a loss of funds.

Recommended Mitigation

In the price() function of VotiumStrategy.sol, consider adding a parameter that determines if the response from Chainlink's price feed is validated:

VotiumStrategy.sol#L31-L33

-   function price() external view override returns (uint256) {
-       return (cvxPerVotium() * ethPerCvx(false)) / 1e18;
+   function price(bool _validate) external view override returns (uint256) {
+       return (cvxPerVotium() * ethPerCvx(_validate)) / 1e18;
    }

The price() function in AfEth.sol should then use the validated price of vAfEth in its calculations, and revert if it the price is incorrect:

AfEth.sol#L138-L140

-       uint256 vEthValueInEth = (vEthStrategy.price() *
+       uint256 vEthValueInEth = (vEthStrategy.price(true) *
            vEthStrategy.balanceOf(address(this))) / 1e18;
        return ((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply();

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L133-L141

Vulnerability details

Bug Description

In AfEth.sol, the price() function returns the current price of afEth:

AfEth.sol#L133-L141

    function price() public view returns (uint256) {
        if (totalSupply() == 0) return 1e18;
        AbstractStrategy vEthStrategy = AbstractStrategy(vEthAddress);
        uint256 safEthValueInEth = (ISafEth(SAF_ETH_ADDRESS).approxPrice(true) *
            safEthBalanceMinusPending()) / 1e18;
        uint256 vEthValueInEth = (vEthStrategy.price() *
            vEthStrategy.balanceOf(address(this))) / 1e18;
        return ((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply();
    }

As seen from above, the price of afEth is calculated by the TVL of both safEth and vAfEth divided by totalSupply(). However, this calculation does not take into account afEth that is transferred to the contract when requestWithdraw() is called:

AfEth.sol#L183-L187

        uint256 afEthBalance = balanceOf(address(this));
        uint256 withdrawRatio = (_amount * 1e18) /
            (totalSupply() - afEthBalance);

        _transfer(msg.sender, address(this), _amount);

When a user calls requestWithdraw() to initiate a withdrawal, his afEth is transferred to the AfEth contract as shown above. Afterwards, an amount of vAfEth proportional to his withdrawal amount is burned, and pendingSafEthWithdraws is increased.

When price() is called afterwards, safEthBalanceMinusPending() and vEthStrategy.balanceOf(address(this)) will be decreased. However, since the user's afEth is only transferred and not burnt, totalSupply() remains the same. This causes the value returned by price() to be lower than what it should be, since totalSupply() is larger than the actual circulating supply of afEth.

This is an issue as deposit() relies on price() to determine how much afEth to mint to a depositor:

AfEth.sol#L166-L168

        uint256 amountToMint = totalValue / priceBeforeDeposit;
        if (amountToMint < _minout) revert BelowMinOut();
        _mint(msg.sender, amountToMint);

Where:

• totalValue is the ETH value of the caller's deposit.
• priceBeforeDeposit is the cached value of price().

If anyone has initiated a withdrawal using requestWithdraw() but hasn't called withdraw() to withdraw his funds, price() will be lower than what it should be. Subsequently, when deposit() is called, the depositor will receive more afEth than he should since priceBeforeDeposit is smaller.

Furthermore, a first depositor can call requestWithdraw() with all his afEth immediately after staking to make price() return 0, thereby permanently DOSing all future deposits as deposit() will always revert with a division by zero error.

Impact

When there are pending withdrawals, price() will return a value smaller than its actual value. This causes depositors to receive more afEth than intended when calling deposit(), resulting in a loss of funds for previous depositors.

Additionally, a first depositor can abuse this to force deposit() to always revert, permanently bricking the protocol forever.

Proof of Concept

Assume that the protocol is newly deployed and Alice is the only depositor.

• This means that Alice's afEth balance equals to totalSupply().

Alice calls requestWithdraw() with _amount as all her afEth:

• Since _amount == totalSupply(), withdrawRatio is 1e18 (100%).
• Therefore, all of the protocol's vAfEth is burnt and pendingSafEthWithdraws is increased to the protocol's safEth balance.
• Alice's afEth is transferred to the protocol.

Bob calls deposit() to deposit some ETH into the protocol:

• When price() is called:
  • Since pendingSafEthWithdraws is equal to the protocol's safEth balance, safEthBalanceMinusPending() is 0, therefore safEthValueInEth is also 0.
  • Since vEthStrategy.balanceOf(address(this)) (the protocol's vAfEth balance) is 0, vEthValueInEth is also 0.
  • totalSupply() is non-zero.
  • Therefore, price() returns 0 as:

((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply() = ((0 + 0) * 1e18) / x = 0

• As priceBeforeDeposit is 0, this line reverts with a division by zero error.

As demonstrated above, deposit() will always revert as long as Alice does not call withdraw() to burn her afEth, thereby bricking the protocol's core functionality.

Recommended Mitigation

In price(), consider subtracting the amount of afEth held in the contract from totalSupply():

AfEth.sol#L133-L141

    function price() public view returns (uint256) {
-       if (totalSupply() == 0) return 1e18;
+       uint256 circulatingSupply = totalSupply() - balanceOf(address(this));
+       if (circulatingSupply == 0) return 1e18;
        AbstractStrategy vEthStrategy = AbstractStrategy(vEthAddress);
        uint256 safEthValueInEth = (ISafEth(SAF_ETH_ADDRESS).approxPrice(true) *
            safEthBalanceMinusPending()) / 1e18;
        uint256 vEthValueInEth = (vEthStrategy.price() *
            vEthStrategy.balanceOf(address(this))) / 1e18;
-       return ((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply();
+       return ((vEthValueInEth + safEthValueInEth) * 1e18) / circulatingSupply;
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L233-L240 https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L258-L263

Vulnerability details

Bug Description

In VotiumStrategyCore.sol, the buyCvx() and sellCvx() functions call exchange_underlying() of Curve's ETH / CVX pool to buy and sell CVX respectively:

VotiumStrategyCore.sol#L233-L240

        ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying{
            value: _ethAmountIn
        }(
            0,
            1,
            _ethAmountIn,
            0 // this is handled at the afEth level
        );

VotiumStrategyCore.sol#L258-L263

        ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying(
            1,
            0,
            _cvxAmountIn,
            0 // this is handled at the afEth level
        );

As seen from above, exchange_underlying() is called with its _min_dy parameter as 0, which means the minimum amount of CVX or ETH to receive from the swap is effectively 0.

This isn't an issue when users interact with the AfEth contract, as its deposit() and withdraw() functions include a _minOut parameter which protects against slippage.

However, users that interact with the VotiumStrategy contract directly will not be protected from slippage when they call any of the following functions:

• deposit(), which calls buyCvx()
• depositRewards(), which calls buyCvx()
• withdraw(), which calls sellCvx()

Should users call any of the functions listed above directly, they will be susceptible to sandwich attacks by attackers, which would reduce the amount of CVX or ETH received from the swap with curve's pool.

Impact

Due to a lack of slippage protection in buyCvx() and sellCvx(), users that interact with the VotiumStrategy contract will be susceptible to sandwich attacks. This results in a loss of funds for them as they will receive less CVX or ETH for the same amount of funds.

Proof of Concept

Consider the following scenario:

• Bob calls the VotiumStrategy contract's deposit() function directly to deposit ETH.
• Alice sees his transaction in the mempool and front-runs his transaction. She swaps a large amount of ETH into the Curve pool and gets CVX in return.
• Now, Bob's transaction is executed:
  • buyCvx() attempts to swap Bob's ETH deposit for CVX.
  • However, since the pool currently has a lot more ETH than CVX, Bob only gets a small amount of CVX in return.
• Alice back-runs Bob's transaction and swaps the CVX she gained for ETH in the pool, which would result in a profit for her.

In this scenario, Alice has sandwiched Bob's deposit() transaction for a profit, causing Bob to receive less CVX for his deposited ETH.

Recommended Mitigation

Consider adding a _minOut parameter to either buyCvx() and sellCvx(), or the following functions:

• deposit()
• depositRewards()
• withdraw()

This allows the caller to specify a minimum amount they expect from the swap, which would protect them from slippage.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L272-L293

Vulnerability details

Bug Description

In VotiumStrategyCore.sol, the buyCvx() function calls exchange_underlying() of Curve's ETH / CVX pool to buy CVX:

VotiumStrategyCore.sol#L233-L240

        ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying{
            value: _ethAmountIn
        }(
            0,
            1,
            _ethAmountIn,
            0 // this is handled at the afEth level
        );

As seen from above, exchange_underlying() is called with its _min_dy parameter as 0, which means the minimum amount of CVX that the swap must return is effectively 0.

buyCvx() is used by VotiumStrategyCore's depositRewards() function to swap ETH gained from rewards to CVX:

VotiumStrategyCore.sol#L203-L204

    function depositRewards(uint256 _amount) public payable {
        uint256 cvxAmount = buyCvx(_amount);

This is called by the depositRewards() function in the AfEth contract:

AfEth.sol#L291

            votiumStrategy.depositRewards{value: amount}(amount);

However, the depositRewards() function in AfEth.sol, unlike deposit() and withdraw(), does not have a _minOut parameter or any other form of slippage protection. This leaves it susceptible to sandwich attacks.

Impact

As the depositRewards() function in AfEth.sol does not have any form of slippage protection, attackers can sandwich calls to depositRewards() to extract value from its buyCvx() call, resulting in a loss of rewards for the protocol.

Note that depositRewards() is automatically called whenever the rewarder of the VotiumStrategy contract calls applyRewards().

Proof of Concept

Consider the following scenario:

• Rewarder calls applyRewards() to distribute rewards to users. This calls depositRewards() in AfEth.sol to deposit ETH and exchange it to CVX.
• Alice sees his transaction in the mempool and front-runs it. She swaps a large amount of ETH into the Curve pool and gets CVX in return.
• Now, the rewarder's transaction is executed:
  • This calls depositRewards() in the VotiumStrategy contract with a portion of the deposited ETH.
  • buyCvx() attempts to swap the deposited ETH for CVX.
  • However, since the pool currently has a lot more ETH than CVX, buyCvx() only returns a small amount of CVX when called.
• Alice back-runs the transaction and swaps the CVX she gained back into ETH, which results in a profit for her.

In this scenario, Alice has sandwiched the call to depositRewards() to effectively steal a portion of rewards from the protocol.

Recommended Mitigation

Consider adding some form of slippage protection to depositRewards() in AfEth.sol. One way of achieving this would be to do the following:

1. Return cxvAmount when depositRewards() in VotiumStrategyCore is called:

VotiumStrategyCore.sol#L203-L208

-   function depositRewards(uint256 _amount) public payable {
-       uint256 cvxAmount = buyCvx(_amount);
+   function depositRewards(uint256 _amount) public payable returns (uint256 cvxAmount) {
+       cvxAmount = buyCvx(_amount);
        IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
        ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
        emit DepositReward(cvxPerVotium(), _amount, cvxAmount);
    }

2. Add a _minOut parameter in depositRewards(), which is the minimum amount of CVX that should be returned by the swap:

AfEth.sol#L272-L293

-   function depositRewards(uint256 _amount) public payable {
+   function depositRewards(uint256 _amount, uint256 _minOut) public payable {
        // Some code here...

        if (safEthRatio < ratio) {
            ISafEth(SAF_ETH_ADDRESS).stake{value: amount}(0);
        } else {
-           votiumStrategy.depositRewards{value: amount}(amount);
+           uint256 cvxAmount = votiumStrategy.depositRewards{value: amount}(amount);
+           if (cvxAmount < _minOut) revert BelowMinOut();
        }
    }

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L145-L148

Vulnerability details

Bug Description

In VotiumStrategy.sol, the relock() function is used to withdraw all unlockable CVX and then lock an appropriate amount of CVX again.

It does so by calling lock() of the VLCVX contract:

VotiumStrategy.sol#L145-L148

        if (cvxAmountToRelock > 0) {
            IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmountToRelock);
            ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmountToRelock, 0);
        }

Where:

• cvxAmountToRelock is the total amount of CVX withdrawn - the amount of CVX used for pending withdrawals.

In the VLCVX contract, it is possible for lock() to revert if isShutdown is set to true:

CvxLockerV2.sol#L1465

    function _lock(address _account, uint256 _amount, uint256 _spendRatio, bool _isRelock) internal {
        require(_amount > 0, "Cannot stake 0");
        require(_spendRatio <= maximumBoostPayment, "over max spend");
        require(!isShutdown, "shutdown");

Note that the contract's owner can call shutdown() to set isShutdown to true.

As such, if isShutdown is set to true, relock() will always revert as it does not check the value of isShutdown before making a call to lock().

relock() is called by the withdraw() function:

VotiumStrategy.sol#L109-L117

    function withdraw(uint256 _withdrawId) external override {
        // Some checks here...

        relock();

Therefore, if VLCVX's owner sets isShutdown to true, the withdraw() function will permanently be DOSed as relock() will always revert.

Impact

Should VLCVX's owner decide to shutdown the contract, the relock() function in VotiumStrategy.sol will always revert. As a result, withdrawals will permanently be DOSed as withdraw() in VotiumStrategy.sol and AfEth.sol call relock() in their logic.

This leads to a loss of funds for users as they will no longer be able to withdraw their ETH using the withdraw() function.

Additionally, since relock() is the only function that withdraws unlockable CVX from the VLCVX contract, all CVX will forever be stuck in as there is no other way to withdraw the locked CVX.

Recommended Mitigation

In relock(), consider calling lock() only when isShutdown is false:

VotiumStrategy.sol#L145-L148

-       if (cvxAmountToRelock > 0) {
+       if (cvxAmountToRelock > 0 && !ILockedCvx(VLCVX_ADDRESS).isShutdown) {
            IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmountToRelock);
            ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmountToRelock, 0);
        }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L181-L184 https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L74-L77

Vulnerability details

Bug Description

In AfEth.sol, whenever a user calls requestWithdraw() to queue a withdrawal, the time that they can withdraw is determined by withdrawTime():

AfEth.sol#L175-L176

    function requestWithdraw(uint256 _amount) external virtual {
        uint256 withdrawTimeBefore = withdrawTime(_amount);

withdrawTime() determines the withdrawal time as follows:

VotiumStrategy.sol#L174-L194

        (
            ,
            uint256 unlockable,
            ,
            ILockedCvx.LockedBalance[] memory lockedBalances
        ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(address(this));
        uint256 cvxAmount = (_amount * _priceInCvx) / 1e18;
        uint256 totalLockedBalancePlusUnlockable = unlockable +
            IERC20(CVX_ADDRESS).balanceOf(address(this));

        for (uint256 i = 0; i < lockedBalances.length; i++) {
            totalLockedBalancePlusUnlockable += lockedBalances[i].amount;
            // we found the epoch at which there is enough to unlock this position
            if (
                totalLockedBalancePlusUnlockable >=
                cvxUnlockObligations + cvxAmount
            ) {
                return lockedBalances[i].unlockTime;
            }
        }
        revert InvalidLockedAmount();

Where:

• unlockable is the amount of CVX that can be unlocked currently.
• lockedBalances stores the amount of CVX locked and unlockTime for each epoch that is still locked.

As seen from above, it iterates through lockedBalances to find an epoch where the amount of CVX unlockable is sufficient for the withdrawal. If an epoch is not found in the for-loop, withdrawTime() will revert.

The same logic is seen in the requestWithdraw() function of VotiumStrategy.sol:

VotiumStrategy.sol#L65-L102

        (
            ,
            uint256 unlockable,
            ,
            ILockedCvx.LockedBalance[] memory lockedBalances
        ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(address(this));
        uint256 cvxAmount = (_amount * _priceInCvx) / 1e18;
        cvxUnlockObligations += cvxAmount;

        uint256 totalLockedBalancePlusUnlockable = unlockable +
            IERC20(CVX_ADDRESS).balanceOf(address(this));

        for (uint256 i = 0; i < lockedBalances.length; i++) {
            totalLockedBalancePlusUnlockable += lockedBalances[i].amount;
            // we found the epoch at which there is enough to unlock this position
            if (totalLockedBalancePlusUnlockable >= cvxUnlockObligations) {
                // Some code that returns here...
            }
        }
        // should never get here
        revert InvalidLockedAmount();

However, this implementation does not check if totalLockedBalancePlusUnlockable contains sufficient CVX before iterating through lockedBalances. Therefore, even if the amount of CVX unlockable is sufficient, users will still be forced to wait until the next locked epoch to withdraw their funds.

Furthermore, if lockedBalances is empty, withdrawTime() and requestWithdraw() will always revert as the for-loop never runs. This could occur if deposit(), relock() and depositRewards() aren't called for a period of time, causing all CVX of the VotiumStrategy contract to be unlockable (ie. there are no epochs where VotiumStrategy still has CVX locked in the VLCVX contract).

Should this occur, the only way for users to queue withdrawals would be to call relock() to withdraw all unlockable CVX and lock them again. However, this would force users to wait the entire lock duration before they can withdraw, which is 16 weeks:

CvxLockerV2.sol#L1007

    // Duration that rewards are streamed over
    uint256 public constant rewardsDuration = 86400 * 7;

    // Duration of lock/earned penalty period
    uint256 public constant lockDuration = rewardsDuration * 16;

Impact

If the amount of unlockable CVX in VotiumStrategy is sufficient for withdrawals, users should be able to instantly withdraw their funds.

However, as withdrawTime() and requestWithdraw() do not check the amount of unlockable CVX first, users are instead forced to wait until the next lock epoch before being able to withdraw. This could even last up to 16 weeks.

Recommended Mitigation

In withdrawTime(), consider returning block.timestamp if totalLockedBalancePlusUnlockables is sufficient:

VotiumStrategy.sol#L181-L184

        uint256 totalLockedBalancePlusUnlockable = unlockable +
            IERC20(CVX_ADDRESS).balanceOf(address(this));

+       if (totalLockedBalancePlusUnlockable >= cvxUnlockObligations + cvxAmount) return block.timestamp;
        for (uint256 i = 0; i < lockedBalances.length; i++) {

The same change should be applied to requestWithdraw() as well:

VotiumStrategy.sol#L74-L77

        uint256 totalLockedBalancePlusUnlockable = unlockable +
            IERC20(CVX_ADDRESS).balanceOf(address(this));

+       if (totalLockedBalancePlusUnlockable >= cvxUnlockObligations) {
+           withdrawIdToWithdrawRequestInfo[latestWithdrawId] = WithdrawRequestInfo({
+               cvxOwed: cvxAmount,
+               withdrawn: false,
+               epoch: withdrawEpoch,
+               owner: msg.sender
+           });
+           return latestWithdrawId;
+       }
        for (uint256 i = 0; i < lockedBalances.length; i++) {

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39-L46

Vulnerability details

Bug Description

In VotiumStrategyCore.sol, the cvxInSystem() function returns the total amount of CVX in the protocol:

VotiumStrategyCore.sol#L133-L138

    function cvxInSystem() public view returns (uint256) {
        (uint256 total, , , ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(
            address(this)
        );
        return total + IERC20(CVX_ADDRESS).balanceOf(address(this));
    }

The total amount of CVX is sum of the amount of locked CVX and the contract's current CVX balance. An attacker can "donate" CVX to the protocol to cause cvxInSystem() to increase by doing either of the following:

1. Directly transfer CVX to the VotiumStrategy contract.
2. Call depositRewards() with ETH, which will exchange ETH for CVX and hold it in the contract.

cvxInSystem() is called by cvxPerVotium() to determine the amount of CVX per vAfEth:

VotiumStrategyCore.sol#L144-L149

    function cvxPerVotium() public view returns (uint256) {
        uint256 supply = totalSupply();
        uint256 totalCvx = cvxInSystem();
        if (supply == 0 || totalCvx == 0) return 1e18;
        return ((totalCvx - cvxUnlockObligations) * 1e18) / supply;
    }

Where:

• cvxUnlockObligations is the amount of CVX for pending withdrawals.

As seen from above, cvxPerVotium() is essentially calculated from cvxInSystem() divided by the total supply of vAfEth. Therefore, if cvxInSystem() increases, cvxPerVotium() will also increase proportionally.

This becomes problematic as cvxPerVotium() is used to determine the amount of vAfEth to mint to the depositor when deposit() is called:

VotiumStrategy.sol#L39-L46

    function deposit() public payable override returns (uint256 mintAmount) {
        uint256 priceBefore = cvxPerVotium();
        uint256 cvxAmount = buyCvx(msg.value);
        IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
        ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
        mintAmount = ((cvxAmount * 1e18) / priceBefore);
        _mint(msg.sender, mintAmount);
    }

The amount of vAfEth to mint is determined by the caller's deposit (cvxAmount) is divided by cvxPerVotium().

However, when the total supply of vAfEth is extremely small, an attacker can artificially inflate cxvPerVotium() by "donating" a huge amount of CVX to the protocol. If priceBefore becomes sufficiently large, cvxAmount * 1e18 / priceBefore will round down to 0, causing subsequent depositors to receive 0 vAfEth.

Impact

When the total supply of the VotiumStrategy contract is extremely small, an attacker can force mintAmount to round down to 0 in deposit(), thereby minting 0 vAfEth to subsequent depositors. This will cause all future deposits to accrue to the attacker's vAfEth, leading to a theft of funds from future depositors.

Proof of Concept

Assume that the VotiumStrategy contract is newly deployed and has no depositors yet.

Alice calls deposit() with an extremely small msg.value:

• buyCvx() returns cvxAmount = 1.
• Since this is the first deposit, cvxPerVotium() is 1e18.
• 1 vAfEth is minted to Alice as:

mintAmount = ((cvxAmount * 1e18) / priceBefore) = ((1 * 1e18) / 1e18) = 1

Alice directly transfers 1000 CVX to the contract. Now, if cvxPerVotium() is called:

• totalSupply() = 1
• cvxInSystem() = 1000e18
• cvxUnlockObligations = 0
• Therefore, it returns 1e36 as:

((totalCvx - cvxUnlockObligations) * 1e18) / supply = ((1000e18 - 0) * 1e18) / 1 = 1e39

Bob calls deposit() with ETH worth 500 CVX:

• buyCvx() returns cvxAmount = 500e18
• As shown above, cvxPerVotium() returns 1e39.
• mintAmount will round down to 0 as:

mintAmount = ((cvxAmount * 1e18) / priceBefore) = ((500e18 * 1e18) / 1e39) = 0

• Therefore, Bob does not receive any vAfEth for his deposit.

In this scenario, mintAmount will always round down to 0 if Bob deposits less than 1000 CVX worth of ETH. All of the CVX deposited by Bob accrues to Alice's 1 vAfEth, resulting in a loss of funds for Bob.

Note that another possible way for totalSupply() to become 1 is if everyone withdraws and Alice happens to be the last depositor in the VotiumStrategy contract. She can then withdraw all her vAfEth except 1 by calling requestWithdraw() and withdraw().

Recommended Mitigation

In deposit(), consider minting x amount of vAfEth to a dead address when totalSupply() == 0:

    function deposit() public payable override returns (uint256 mintAmount) {
+       if (totalSupply() == 0) {
+           _mint(address(0), 1000);
+       }
        
        uint256 priceBefore = cvxPerVotium();
        uint256 cvxAmount = buyCvx(msg.value);
        IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
        ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
        mintAmount = ((cvxAmount * 1e18) / priceBefore);
        _mint(msg.sender, mintAmount);
    }

This ensures that totalSupply() will never be low enough for an attacker to inflate cvxPerVotium() significantly.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L173-L181

Vulnerability details

Bug Description

The ethPerCvx() function relies on a Chainlink oracle to fetch the CVX / ETH price:

VotiumStrategyCore.sol#L158-L169

        try chainlinkCvxEthFeed.latestRoundData() returns (
            uint80 roundId,
            int256 answer,
            uint256 /* startedAt */,
            uint256 updatedAt,
            uint80 /* answeredInRound */
        ) {
            cl.success = true;
            cl.roundId = roundId;
            cl.answer = answer;
            cl.updatedAt = updatedAt;
        } catch {

The return values from latestRoundData() are validated as such:

VotiumStrategyCore.sol#L173-L181

        if (
            (!_validate ||
                (cl.success == true &&
                    cl.roundId != 0 &&
                    cl.answer >= 0 &&
                    cl.updatedAt != 0 &&
                    cl.updatedAt <= block.timestamp &&
                    block.timestamp - cl.updatedAt <= 25 hours))
        ) {

As seen from above, there is no check to ensure that cl.answer does not go below or above a certain price.

Chainlink aggregators have a built in circuit breaker if the price of an asset goes outside of a predetermined price band. Therefore, if CVX experiences a huge drop/rise in value, the CVX / ETH price feed will continue to return minAnswer/maxAnswer instead of the actual price of CVX.

Currently, minAnswer is set to 1e13 and maxAnswer is set to 1e18. This can be checked by looking at the AccessControlledOffchainAggregator contract for the CVX / ETH price feed. Therefore, if CVX ever experiences a flash crash and its price drops to below 1e13 (eg. 100), the cl.answer will still be 1e13.

This becomes problematic as ethPerCvx() is used to determine the price of vAfEth:

VotiumStrategy.sol#L31-L33

    function price() external view override returns (uint256) {
        return (cvxPerVotium() * ethPerCvx(false)) / 1e18;
    }

Furthermore, vAfEth's price is used to calculate the amount of AfEth to mint to users whenever they call deposit():

AfEth.sol#L162-L166

        totalValue +=
            (sMinted * ISafEth(SAF_ETH_ADDRESS).approxPrice(true)) +
            (vMinted * vStrategy.price());
        if (totalValue == 0) revert FailedToDeposit();
        uint256 amountToMint = totalValue / priceBeforeDeposit;

If CVX experiences a flash crash, vStrategy.price() will be 1e13, which is much larger than the actual price of CVX. This will cause totalValue to become extremely large, which in turn causes amountToMint to be extremely large as well. Therefore, the caller will receive a huge amount of afEth.

Impact

Due to Chainlink's in-built circuit breaker mechanism, if CVX experiences a flash crash, ethPerCvx() will return a price higher than the actual price of CVX. Should this occur, an attacker can call deposit() to receive a huge amount of afEth as it uses an incorrect CVX price.

This would lead to a loss of funds for previous depositors, as the attacker would hold a majority of afEth's total supply and can withdraw most of the protocol's TVL.

Proof of Concept

Assume the following:

• For convenience, assume that 1 safEth is worth 1 ETH.
• The AfEth contract has the following state:
  • ratio = 5e17 (50%)
  • totalSupply() = 100e18
  • safEthBalanceMinusPending() = 50e18
  • vEthStrategy.balanceOf(address(this)) (vAfEth balance) is 50e18
• The VotiumStrategy contract has the following state:
  • Only 50 vAfEth has been minted so far (totalSupply() = 50e18).
  • The contract only has 50 CVX in total (cvxInSystem() = 50e18).
  • This means that cvxPerVotium() returns 1e18 as:

((totalCvx - cvxUnlockObligations) * 1e18) / supply = ((50e18 - 0) * 1e18) / 50e18 = 1e18

The price of CVX flash crashes from 2e15 / 1e18 ETH per CVX to 100 / 1e18 ETH per CVX. Now, if an attacker calls deposit() with 10 ETH:

• priceBeforeDeposit, which is equal to price(), is 5e17 + 5e12 as:

safEthValueInEth = (1e18 * 50e18) / 1e18 = 50e18
vEthValueInEth = (1e13 * 50e18) / 1e18 = 5e14
((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply() = ((50e18 + 5e14) * 1e18) / 100e18 = 5e17 + 5e12

• Since ratio is 50%, 5 ETH is staked into safEth:
  • sMinted = 5e18, since the price of safEth and ETH is equal.
• The other 5 ETH is deposited into VotiumStrategy:
  • priceBefore, which is equal to cvxPerVotium(), is 1e18 as shown above.
  • Since 1 ETH is worth 1e16 CVX (according to the price above), cvxAmount = 5e34.
  • Therefore, vMinted = 5e34 as:

mintAmount = ((cvxAmount * 1e18) / priceBefore) = ((5e34 * 1e18) / 1e18) = 5e34 

• To calculate vStrategy.price() after VotiumStrategy's deposit() function is called:
  • ethPerCvx() returns 1e13, which is minAnswer for the CVX / ETH price fee.
  • cvxPerVotium() is still 1e18 as:

supply = totalSupply() = 5e34 + 50e18
totalCvx = cvxInSystem() = 5e34 + 50e18
((totalCvx - cvxUnlockObligations) * 1e18) / supply = ((5e34 + 50e18 - 0) * 1e18) / (5e34 + 50e18) = 1e18

• Therefore vStrategy.price() returns 1e13 as:

(cvxPerVotium() * ethPerCvx(false)) / 1e18 = (1e18 * 1e13) / 1e18 = 1e13

• To calculate the amount of AfEth minted to the caller:

totalValue = (5e18 * 1e18) + (5e34 * 1e13) = 5e47 + 5e36
amountToMint = totalValue / priceBeforeDeposit = (5e47 + 5e36) / (5e17 + 5e12) = ~1e30

As seen from above, the attacker will receive 1e30 AfEth, which is huge compared to the remaining 100e18 held by previous depositors before the flash crash.

Therefore, almost all of the protocol's TVL now belongs to the attacker as he holds most of AfEth's total supply. This results in a loss of funds for all previous depositors.

Recommended Mitigation

Consider validating that the price returned by Chainlink's price feed does not go below/above a minimum/maximum price:

VotiumStrategyCore.sol#L173-L181

        if (
            (!_validate ||
                (cl.success == true &&
                    cl.roundId != 0 &&
-                   cl.answer >= 0 &&
+                   cl.answer >= MIN_PRICE &&
+                   cl.answer <= MAX_PRICE &&
                    cl.updatedAt != 0 &&
                    cl.updatedAt <= block.timestamp &&
                    block.timestamp - cl.updatedAt <= 25 hours))
        ) {

This ensures that an incorrect price will never be used should CVX experience a flash crash, thereby protecting the assets of existing depositors.

Assessed type

Oracle

Findings Summary

[L-01] withdraw() in AfEth.sol can be called with the same _withdrawId multiple times

In AfEth.sol, withdrawals are a two-step process. requestWithdraw() is called first to initiate the withdrawal process, which calculates how much safEth and CVX should be withdrawn based on the amount of afEth the user specified and assigns a _withdrawId.

After a period of time, the user can then call withdraw() with their _withdrawId to withdraw their funds:

AfEth.sol#L243-L250

    function withdraw(
        uint256 _withdrawId,
        uint256 _minout
    ) external virtual onlyWithdrawIdOwner(_withdrawId) {
        if (pauseWithdraw) revert Paused();
        uint256 ethBalanceBefore = address(this).balance;
        WithdrawInfo memory withdrawInfo = withdrawIdInfo[_withdrawId];
        if (!canWithdraw(_withdrawId)) revert CanNotWithdraw();

withdraw() checks if the _withdrawId given is ready for withdrawal using canWithdraw():

VotiumStrategy.sol#L155-L163

    function canWithdraw(
        uint256 _withdrawId
    ) external view virtual override returns (bool) {
        uint256 currentEpoch = ILockedCvx(VLCVX_ADDRESS).findEpochId(
            block.timestamp
        );
        return
            withdrawIdToWithdrawRequestInfo[_withdrawId].epoch <= currentEpoch;
    }

canWithdraw() only checks if a sufficient amount of time has passed since requestWithdraw() was called, and doesn't check if withdraw() has already been called for the given _withdrawId.

Therefore, withdraw() can be called multiple times with the same _withdrawId, which would allow the user to withdraw more funds than originally allocated in requestWithdraw().

This is currently unexploitable as VotiumStrategy's withdraw() function reverts when called with the same _withdrawId more than once.

However, if setStrategyAddress() is ever called to swap out the VotiumStrategy contract with a new strategy, and the new strategy does not check _withdrawId, this could become exploitable.

Recommendation

Ensure that withdraw() cannot be called with the same _withdrawId multiple times in AfEth.sol.

[L-02] depositRewards() might make the safEth:vAfEth allocation even further from ratio

In depositRewards(), the ETH transferred to the contract is deposited into either safEth or vAfEth based on the following logic:

AfEth.sol#L286-L292

        uint256 totalTvl = (safEthTvl + votiumTvl);
        uint256 safEthRatio = (safEthTvl * 1e18) / totalTvl;
        if (safEthRatio < ratio) {
            ISafEth(SAF_ETH_ADDRESS).stake{value: amount}(0);
        } else {
            votiumStrategy.depositRewards{value: amount}(amount);
        }

As seen from above, it calculates the safEthRatio using safEth's TVL divided by overall TVL, and checks if safEthRatio is below ratio.

However, such an implementation could actually bring the safEth:vAfEth ratio further away from ratio if the difference between safEthRatio and ratio is small, and the amount of ETH to deposit is large.

For example, if ratio - safEthRatio = 1, the safEth:vAfEth ratio is effectively already equal to ratio. However, due to the logic above, all ETH will be deposited into safEth, which would increase safEthRatio to become much higher than ratio.

Recommendation

Consider splitting the amount of ETH to deposit between safEth and vAfEth based on ratio, and deposit into both of them whenever depositRewards() is called (similar to deposit()).

This would ensure that the safEth:vAfEth ratio always averages towards ratio whenever depositRewards() is called.

[L-03] Not having a backup price oracle could DOS depositRewards()

The ethPerCvx() is used to determine the price of CVX in terms of ETH using a Chainlink price oracle. If it is called with _validate = true, the values returned from Chainlink's price feed are validated as such:

VotiumStrategyCore.sol#L173-L185

        if (
            (!_validate ||
                (cl.success == true &&
                    cl.roundId != 0 &&
                    cl.answer >= 0 &&
                    cl.updatedAt != 0 &&
                    cl.updatedAt <= block.timestamp &&
                    block.timestamp - cl.updatedAt <= 25 hours))
        ) {
            return uint256(cl.answer);
        } else {
            revert ChainlinkFailed();
        }

As seen from above, if Chainlink's oracle returns a stale price, ethPerCvx() will simply revert as it does not have a backup price oracle.

This could DOS the depositRewards() function, which calls ethPerCvx(true):

AfEth.sol#L283-L284

        uint256 votiumTvl = ((votiumStrategy.cvxPerVotium() *
            votiumStrategy.ethPerCvx(true)) *

Therefore, if Chainlink's price oracle ever returns stale prices, depositRewards() will be DOSed as ethPerCvx() will always revert.

Recommendation

Consider implementing a secondary price oracle that can be queried for prices in ethPerCvx() should Chainlink's oracle go down or become stale.

This ensures that depositRewards() can still be called even when Chainlink's oracles are down.

[L-04] Freshness threshold should not be a hardcoded value

The ethPerCvx() function checks if the price returned from Chainlink's oracle is stale as follows:

VotiumStrategyCore.sol#L180

                    block.timestamp - cl.updatedAt <= 25 hours))

Where:

• cl.updatedAt is the value of updatedAt returned from latestRoundData().

As seen from above, it checks if more than 25 hours has passed since cl.updatedAt, and reverts if so. This works for the current price oracle, which has a heartbeat of 24 hours.

However, it is possible for the oracle's heartbeat to be updated to a different time period (eg. 3 hours), or if setChainlinkCvxEthFeed() is called to update the oracle to a new one that has a different heartbeat:

VotiumStrategyCore.sol#L76-L80

    function setChainlinkCvxEthFeed(
        address _cvxEthFeedAddress
    ) public onlyOwner {
        chainlinkCvxEthFeed = AggregatorV3Interface(_cvxEthFeedAddress);
    }

If the oracle's heartbeat becomes shorter, such as 3 hours, checking for staleness with a freshness threshold of 25 hours is essentially useless, since the oracle prices will already be stale if more than 3 hours have passed.

On the other hand, if the oracle's heartbeat becomes longer, such as 48 hours, a freshness threshold of 25 hours would incorrectly DOS the ethPerCvx() function since it will revert even when the price isn't stale.

Therefore, having a hardcoded freshness threshold of 25 hours is problematic as it cannot be updated should the oracle's heartbeat change.

Recommendation

Consider checking for stale prices using a FRESHNESS_THRESHOLD state variable, which can be modified by the contract's owner:

VotiumStrategyCore.sol#L180

-                   block.timestamp - cl.updatedAt <= 25 hours))
+                   block.timestamp - cl.updatedAt <= FRESHNESS_THRESHOLD))

[L-05] Users who hold vAfEth instead of afETh might lose out on rewards unfairly

In the applyRewards() function, the ETH gained by selling rewards from Votium is deposited into depositRewards() of the AfEth contract:

VotiumStrategyCore.sol#L302-L304

        if (address(manager) != address(0))
            IAfEth(manager).depositRewards{value: ethReceived}(ethReceived);
        else depositRewards(ethReceived);

depositRewards() in the AfEth contract determines deposits the ETH into either safEth or back into the VotiumStrategy contract based on the current safEthRatio:

AfEth.sol#L286-L292

        uint256 totalTvl = (safEthTvl + votiumTvl);
        uint256 safEthRatio = (safEthTvl * 1e18) / totalTvl;
        if (safEthRatio < ratio) {
            ISafEth(SAF_ETH_ADDRESS).stake{value: amount}(0);
        } else {
            votiumStrategy.depositRewards{value: amount}(amount);
        }

However, if the ETH gained from rewards are deposited into safEth, users that are holding vAfEth will not accrue any rewards. This is because vAfEth's TVL is only affected by the protocol's amount of CVX, and not the protocol's safEth amount, which means that the price of vAfEth will not change if rewards are deposited into safEth.

Therefore, if applyRewards() deposits rewards into safEth, only afEth holders will accrue rewards, leading to an unfair loss of yield for users that hold vAfEth.

Note that it is possible for users to hold vAfEth by calling the deposit() function of the VotiumStrategy contract directly, instead of depositing into the AfEth contract.

Recommendation

In applyRewards(), consider depositing rewards to only the VotiumStrategy contract:

VotiumStrategyCore.sol#L302-L304

-       if (address(manager) != address(0))
-           IAfEth(manager).depositRewards{value: ethReceived}(ethReceived);
-       else depositRewards(ethReceived);
+       depositReward(ethReceived);