Asymmetry Finance afETH Invitational - m_Rassska's results

Lines of code

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

Vulnerability details

Vulnerability Details

• Upon claiming Votium rewards, applyRewards() is intended to be invoked in order to exchange the tokens for eth and put the eth received back into the strategies. Based on the current ratio it either stakes the amount into safETH or obtains some CVX by selling eth on Curve and then locks them to get vlCVX.

• Since afETH.depositRewards() or VotiumStrategy.depositRewards() can be called by anyone, an adversary is able to manipulate the CVX/ETH pool in such a way that the CVX tokens will be bought at inflated rates creating an arbitrage opportunity for an adversary.

Impact

• It's possible to steal any eth hold by afETH or VotiumStrategy.

  • But when exactly afETH or VotiumStrategy will hold some eth apart from accidental eth transfers? This has been discussed with the sponsors and we have the following context:

    • Possibility to boost user rewards by dropping additional eth and re-investing them after the rewards being received. The time difference between additional eth being dropped and rewards from Votium being claimed is exactly what an adversary needs to steal that additional eth.

    • In a case, where the ratio > safEthRatio rewarder oracle will not be able to invoke applyRewards() after rewards being claimed due to the min deposit amount limits setted by safETH, which ultimately results in tx reversion.

    • Upon further supporting more strategies.

• Marking this as medium severity, since the impact is critical, but the likelihood is low/medium.

Proof of Concept

• Steps to execute such attack are simple:
  • Borrow an ETH on Balancer to make the slippage significant in CVX/ETH curve pool.
  • In the same tx, invoke afETH.depositRewards()/VotiumStrategy.depositRewards() while the rates are inflated.
  • Rebalance the pool back by taking out an ETH.
  • Repay flash loan fee and drop that additional dirty ETH into favorite mixer.

Recommended Mitigation Steps

• Short term:
  • Make depositRewards() being invoked internally only by underlying strategies.
• Long term:
  • Probably, it's possible to compare the amount received from the trade against chainlink price feed's answer. If the amount received deviates from oracle's answer more than x%, the whole tx will be reverted. Basically, setting the slippage control by utilizing price oracles.

Assessed type

Context

Lines of code

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

Vulnerability details

Vulnerability Details

• The totalLockedBalancePlusUnlockable is being used to calculate an amount that's ready to be withdrawn. In case, if totalLockedBalancePlusUnlockable >= cvxUnlockObligations already before iterating over the lockedBalances, the withdrawal request is intended to be finalized right away, since the main criteria by design is whether the system has enough unlocked liquidity to finalize the request or not. If not, then it has to iterate and search for a locked position in order to cover uncovered portion of a withdrawal being requested.

Impact

• Possible balance lock in the future, but very unlikely, since it requires locker being shutdown or some additional migrations(while adding more strategies), which forces Asymmetry to set the ratio = 1e18

Recommended Mitigation Steps

• Short term:
  • I'm not 100% sure about this, since it looks like a crutch:

    function requestWithdraw(
        uint256 _amount
    ) public override returns (uint256 withdrawId) {
        ...
        cvxUnlockObligations += cvxAmount;
    
        uint256 totalLockedBalancePlusUnlockable = unlockable +
            IERC20(CVX_ADDRESS).balanceOf(address(this));
    
      ++  if (totalLockedBalancePlusUnlockable>=cvxUnlockObligations) {
      ++    uint256 withdrawEpoch = currentEpoch;
      ++    withdrawIdToWithdrawRequestInfo[
      ++        latestWithdrawId
      ++    ] = WithdrawRequestInfo({
      ++        cvxOwed: cvxAmount,
      ++        withdrawn: false,
      ++        epoch: withdrawEpoch,
      ++        owner: msg.sender
      ++    });
      ++
      ++    emit WithdrawRequest(msg.sender, cvxAmount, latestWithdrawId);
      ++    return latestWithdrawId;
      ++  }
    
        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) {
                (, uint32 currentEpochStartingTime) = ILockedCvx(VLCVX_ADDRESS)
                    .epochs(currentEpoch);
                uint256 timeDifference = lockedBalances[i].unlockTime -
                    currentEpochStartingTime;
                uint256 epochOffset = timeDifference /
                    ILockedCvx(VLCVX_ADDRESS).rewardsDuration();
                uint256 withdrawEpoch = currentEpoch + epochOffset;
                withdrawIdToWithdrawRequestInfo[
                    latestWithdrawId
                ] = WithdrawRequestInfo({
                    cvxOwed: cvxAmount,
                    withdrawn: false,
                    epoch: withdrawEpoch,
                    owner: msg.sender
                });
    
                emit WithdrawRequest(msg.sender, cvxAmount, latestWithdrawId);
                return latestWithdrawId;
            }
        }
        // should never get here
        revert InvalidLockedAmount();
    }
    

• Long term: Support on architectural level is required:
  • The current assumes are being made towards invoking relock() manually, but what if that hasn't been invoked and the withdrawal request has been submitted which could be covered with the current balance without any unlocks needed?

Assessed type

Context

Lines of code

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

Vulnerability details

Vulnerability Details

• Upon claiming Votium rewards, applyRewards() is intended to be invoked bi-weekly in order to exchange the tokens for eth and put the eth received back into the strategies. Based on the current ratio it either stakes the amount into safETH or obtains some CVX by selling eth on Curve and then locks them to get vlCVX.

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

• Let's say the safEthRatio < ratio, which triggers ISafEth(SAF_ETH_ADDRESS).stake{value: amount} being invoked. And if the amount < ISafEth(SAF_ETH_ADDRESS).minAmount. The whole re-investing strategy collapses.

• As of Sep. 2023, the Votium rewards are 0,000016 eth for 1 vlCVX per round, it means, we need at least 3125 vlCVX being delegated to the Votium in order to pass the threshold.

Impact

• This could backfire in early stages or during certain market conditions, where the amount of vlCVX hold by afETH is not enough to generate 0.05 eth bi-weekly. Basically, that forces to flows that are inconsistent with the main re-investment flow proposed by Asymmetry, which ultimately could result in theft as demonstrated in the issue #15.

Recommended Mitigation Steps

• Short term:
  • Wrapp IAfEth(manager).depositRewards into the try/catch block. And if one of the following conditions arises:
    • The min safETH deposit amount is not reached
    • Chainlink price feed could not be validated
    • Low-level call which sends fees fails
    just simply invoke VotiumStrategy.depositRewards().

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

• Long term: N/A

Assessed type

Context

Overall Analysis

• About the Protocol

  Asymmetry Finance provides an opportunity for stakers to diversify their staked eth across many liquid staking derivatives. It's not a doubt that the Lido has about 80% of the liquid staking market and Asymmetry Finance introduces a great solution to make the LSM more decentralized.

</br>

• Low Severity Issues

  • [L-01] Define the tolerance range for ratio

</br>

• Non-Critical Severity Issues

  • [N-01] Missing events during significant state var changes
  • [N-02] Remove unused imports

</br>

[L-01] Define the tolerance range for ratio<a name="L-01"></a>

• During the afETH initialization, the ratio could be setted to any number. Instead, validate it before setting. Make sure, safETH.minAmount doesn't backfire, when users start to deposit.

Example of an occurance:

• Could be implied here:

</br>

[N-01] Missing events during significant state vars changes<a name="N-01"></a>

• It's pretty crucial to emit the events upon state var changes. It helps for backend services to better handle new conditions.

Example of an occurance:

• Could be added, for instances, here:

</br>

[N-02] Remove unused imports<a name="N-02"></a>

Example of an occurance:

• Could be removed, for instance, here:

</br>

Table of contents

Gas improvement submissions

• [G-01] Cache the state variables into the memory
• [G-02] Cache only attributes needed, when caching an object
• [G-03] Caching global variables is more expensive than using the actual variable (use msg.sender/msg.value instead of caching it
• [G-04] Use nested if statements instead of &&
• [G-05] Use Assembly To Check For address(0)
• [G-06] Internal/private functions only called once can be inlined to save gas
• [G-07] With assembly, .call (bool success) transfer can be done gas-optimized
• [G-08] Duplicated require()/if() checks should be refactored to a modifier or function
• [G-09] Use a hardcoded address instead of address(this)
• [G-10] >= costs less gas than >
• [G-11] Use assembly to emit events
• [G-12] Use constants instead of type(uintX).max
• [G-13] Use assembly for setting stare variables
• [G-14] Unused named return values

[G-01] Cache the state variables into the memory<a name="G-01"></a>

Description:

• mload costs only 3 units of gas, sload(warm access) is about 100 units. Therefore, cache, when it's possible, otherwise - a lot of intrinsic gas wasted for users.

Example of an occurance:

• ratio and SAF_ETH_ADDRESS could be cached here:
• latestWithdrawId after an increment and vEthAddress could be cached here:
• use withdrawInfo.amount here:
• vEthAddress, SAF_ETH_ADDRESS could be cached here:
• VLCVX_ADDRESS could be cached here:
• VLCVX_ADDDRESS, latestWithdrawId, cvxUnlockObligations could be cached outside of the loop here:
• cvxUnlockObligations + cvxAmount could be cached before the loop here:
• VLCVX_ADDRESS, CVX_ADDRESS could be cached here:

</br>

[G-02] Cache only attributes needed, when caching an object<a name="G-02"></a>

Description:

• When caching the struct behind the ref, do not cache the whole struct, if you don't need an access to the whole struct.

Example of an occurance:

• Only 3 of 5 attributes are being used, when caching withdrawInfo, it means we have 2 sloads which incur an overhead here:

</br>

[G-03] Caching global variables is more expensive than using the actual variable (use msg.sender/msg.value instead of caching it<a name="G-03"></a>

Example of an occurance:

• There is no need to store msg.value here:

</br>

[G-04] Use nested if statements instead of &&<a name="G-04"></a>

Description:

• If the “if” statement has a logical “AND” and is not followed by an “else” statement, it can be replaced with 2 if statements.

Example of an occurance:

• Could be optimized here:

</br>

[G-05] Use Assembly To Check For address(0)<a name="G-05"></a>

Description:

• It’s generally more gas-efficient to use assembly to check for a zero address (address(0)) than to use the == operator.

• The reason for this is that the == operator generates additional instructions in the EVM bytecode, which can increase the gas cost of your contract. By using assembly, you can perform the zero address check more efficiently and reduce the overall gas cost of your contract.

• Here’s an example of how you can use assembly to check for a zero address:

  •   contract MyContract {
        function isZeroAddress(address addr) public pure returns (bool) {
            uint256 addrInt = uint256(addr);
            
            assembly {
                // Load the zero address into memory
                let zero := mload(0x00)
                
                // Compare the address to the zero address
                let isZero := eq(addrInt, zero)
                
                // Return the result
                mstore(0x00, isZero)
                return(0, 0x20)
            }
        }
      }

• In the above example, we have a function isZeroAddress that takes an address as input and returns a boolean value, indicating whether the address is equal to the zero address. Inside the function, we convert the address to an integer using uint256(addr), and then use assembly to compare the integer to the zero address.

• By using assembly to perform the zero address check, we can make our code more gas-efficient and reduce the overall cost of our contract. It’s important to note that assembly can be more difficult to read and maintain than Solidity code, so it should be used with caution and only when necessary

Example of an occurance:

• Could be used here:

</br>

[G-06] Internal/private functions only called once can be inlined to save gas<a name="G-06"></a>

Example of an occurance:

• claimVLCVxRewards() and claimVotiumRewards() could be inlined during rewards claiming here:

</br>

[G-07] With assembly, .call (bool success) transfer can be done gas-optimized<a name="G-07"></a>

Description:

• return data (bool success,) has to be stored due to EVM architecture, but in a usage like below, ‘out’ and ‘outsize’ values are given (0,0), this storage disappears and gas optimization is provided.

• Ref: https://twitter.com/pashovkrum/status/1607024043718316032?t=xs30iD6ORWtE2bTTYsCFIQ&s=19

Example of an occurance:

• Could be optimized here:
• Could be optimized here:
• Could be optimized here:

</br>

[G-08] Duplicated require()/if() checks should be refactored to a modifier or function<a name="G-08"></a>

Description:

• Sign modifiers or functions can make your code more gas-efficient by reducing the overall number of operations that need to be executed. For example, if you have a complex validation check that involves multiple operations and you refactor it into a function, then the function can be executed with a single opcode, rather than having to execute each operation separately in multiple locations.

Example of an occurance:

• Could be optimized here:

</br>

[G-09] Use a hardcoded address instead of address(this)<a name="G-09"></a>

Description:

• It can be more gas-efficient to use a hardcoded address instead of the address(this) expression, especially if you need to use the same address multiple times in your contract.

The reason for this is that using address(this) requires an additional EXTCODESIZE operation to retrieve the contract’s address from its bytecode, which can increase the gas cost of your contract. By pre-calculating and using a hardcoded address, you can avoid this additional operation and reduce the overall gas cost of your contract.

Here’s the ref, how to procompute it: https://book.getfoundry.sh/reference/forge-std/compute-create-address

Example of an occurance:

• Could be optimized as an example here:

</br>

[G-10] >= costs less gas than ><a name="G-10"></a>

Description:

• The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas.

Example of an occurance:

• Could be optimize as an example by adding if (amountToMint <= _minout + 1)here:

</br>

[G-11] Use assembly to emit events<a name="G-11"></a>

Description:

• We can use assembly to emit events efficiently by utilizing scratch space and the free memory pointer. This will allow us to potentially avoid memory expansion costs. Note: In order to do this optimization safely, we will need to cache and restore the free memory pointer.

• For example, for a generic emit event for eventSentAmountExample:

•     // uint256 id, uint256 value, uint256 amount
      emit eventSentAmountExample(id, value, amount);

• We can use the following assembly emit events:

  •    assembly {
            let memptr := mload(0x40)
            mstore(0x00, calldataload(0x44))
            mstore(0x20, calldataload(0xa4))
            mstore(0x40, amount)
            log1(
                0x00,
                0x60,
                // keccak256("eventSentAmountExample(uint256,uint256,uint256)")
                0xa622cf392588fbf2cd020ff96b2f4ebd9c76d7a4bc7f3e6b2f18012312e76bc3
            )
            mstore(0x40, memptr)
        }

Example of an occurance:

• Could be optimized here:

</br>

[G-12] Use constants instead of type(uintX).max<a name="G-12"></a>

Description:

• It’s generally more gas-efficient to use constants instead of type(uintX).max when you need to set the maximum value of an unsigned integer type.

The reason for this, is that the type(uintX).max expression involves a computation at runtime, whereas a constant is evaluated at compile-time. This means, that using type(uintX).max can result in additional gas costs for each transaction that involves the expression.

By using a constant instead of type(uintX).max, you can avoid these additional gas costs and make your code more efficient.

Example of an occurance:

• Could be optimize as an example here:

</br>

[G-13] Use assembly for setting stare variables<a name="G-13"></a>

Example of an occurance:

• Could be optimize as an example here:

</br>

[G-14] Unused named return values<a name="G-14"></a>

Description:

• If the named return value has been declared, it should be use in order to get intrinsic gas save, otherwise - waste of the gas during deployment.

Example of an occurance:

• For instance, withdrawId is not being used here:

Analysis - Asymmetry AfETH

Summary

Asymmetry Finance provides an opportunity for stakers to diversify their staked eth across many liquid staking derivatives. It's not a doubt that the Lido has about 80% of the liquid staking market and Asymmetry Finance introduces a great solution to make the LSM more decentralized.

• The codebase provided for this audit introduces new mechanism under AfEth, which is by design collateralized by 2 underlying "strategy tokens" in an adjustable ratio:
  • safETH, which is the token over the LSD market (has been audited in previous contest)
  • vAfEth, which utilizes the rewards by locking cvx tokens and delegating the locked positions to the Votium in order for the Votium to decide the gauges to vote for.

</br>

Approach being taken while auditing

• Understanding the codebase through the test coverage and provided docs
• Debugging (line by line) test cases that are pretty crucial in a system
• Generating Invariants with an intention to put the system into unexpected state
• Receiving the context needed from sponsors in order to avoid the future problems
• Testing locally && Reporting

</br>

Architectural high-level overview

Current flow used for deposits

• if the image has not being rendered, see it here

Current flow used for withdrawals

• if the image has not being rendered, see it here

</br>

Technical overview

Current flow used for deposits

• Description:

  • Any user has an ability to deposit eth into AfETH manager. In response, AfEth mints corresponding amount of shares representing the stake being made by user. AfEth shares themselves are minted above the shares minted by underlying strategies based on the ratio. The initial ratio has to be setted at 7e17, meaning 70% of deposited eth will be staked in safETH and only 30% converts into VotiumStrategy shares.

• Potential Improvements:

  • The current flow with ratio mechanism included looks flexible enough to expand over new strategies in the future. However, the manual adjusment of the ratio seems not to be comfortable to deal with, since every single strategy also has its own deposit limits that could be changed in any time, especially when new strategies will be introduced.

Current flow used for withdrawals

• Description:

  • Any user has an ability to withdraw eth from AfETH manager. In order to do that, the user first has to request withdrawal by providing his AfEth shares and second - withdraw the finalized request. In order to finalize the request, the system has to unstake the shares in each strategy currently being used in a system. Since, some strategies do not provide an instant unlock, the manager also has to wait that finalization period, before providing eth back to the user.

• Potential Improvements:

  • The withdrawal queue being used by Lido to process withdrawals has a lot of interesting ideas to consider. I think, the current architecture behind afEth withdrawals could be improved, although it's decent enough.

</br>

Invariants Generated

• The following list includes some manually generated invariants failed, which have been used during an audit to report the issues:

  • afETH should return the correct withdrawTimeBefore, when _amount in afETH has been provided
  • chainlink oracle should return the fresh rates for the pair
  • CVX/ETH pool manipulation should not provide any oppotunities for an adversory to exploit the system
  • applyRewards() has to be called right after claiming the votium rewards
  • requestWithdraw() should check whether the current balance can cover the request being submitted
  • upon requesting withdrawal, the system has to freeze it's balance to prevent double accounting.

• Ofc, the list could contain some unsuccessfull scenarios being generated(>100), but at this point, it doesn't make any sense.

</br>

Centralization risks

• The AfEth manager itself is upgradable, which provides an ability for owner to update the logic behind the manager.
• The percentage of the rewards could be setted up to 100%
• The withdrawals could be paused at any time by an owner.
• Many more, but users also should consider that the current configs setted by Asymmetry could also save the funds in case of any exploit and etc...

Time Spent

• 70 hours, 7 days, 10 hours each!!!

Additional notes

• Special thanks to the team behind Asymmetry for providing the context needed to understand certain conditions better. I've had the pleasure of working with you over this period!

Time spent:

70 hours