Asymmetry Finance afETH Invitational - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

AfEth withdrawing will not work when ratio will be 0. It will be not possible to withdraw.

Proof of Concept

Any ratio for the 2 tokens of afEth can be set by owner. AfEth.withdraw function will not work in case if ratio will be 0, which means that safEth percentage is 0. This is because, safEth doesn't allow to unstake 0 amount.

Also, when ratio is e18, that means that vEth percentage is 0, but requestWithdraw function will still calculate withdraw time, which is not needed in this case and withdraw will be called.

Tools Used

VsCode

Recommended Mitigation Steps

First of all, calculate withdraw time only for vEth amount, not whole amount. Then in case if ratio is e18, then you don't need to do that. Also don't unstake/withdraw 0 shares for safEth and vEth.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Stale cvx price can be used while depositing

Proof of Concept

When user deposits, then price of afEth token is calculated. It's needed to know how many tokens user will receieve.

This price consists of safEth price and vEth price.

This is how price is found for vEth. https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L31-L33

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

Here, ethPerCvx function is called and false is passed as param. This param tells function if it's needed to validate chainlink price. In case if it's false then there will be no validation and as result there is a risk that chainlink will return stale price. As user also provides slippage that actually doesn't mean that he will not be affected, it's still possible that wrong price will fit slippage.

Tools Used

VsCode

Recommended Mitigation Steps

Validate chainlink data in this case.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Attacker can mint afEth with cheaper price and then withdraw.

Proof of Concept

When user would like to buy some amount of afEth tokens, then price is calculated. This price will be used to calculate amount of tokens to mint.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/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();
    }

This is how price is calculated. First, function calculates eth amount that contract holds in safEth tokens. You can see, that safEthBalanceMinusPending function is used to get amount. This function doesn't include safEth that is requested to withdraw. And then vEth amount in terms of eth is calculated. And this total amount then divided by whole totalSupply.

Now, we need to look, how withdraw requests work. First important this is that afEth, sent by user, isn't burnt, but transferred to the contract, which means that totalSupply remains same after request. But, when vEth is requested to withdraw, then it shares are burnt at the moment of request. Also pendingSafEthWithdraws is increased with safEth amount.

This means that when withdraw request is done, then eth amount of safEth token and vEth token has decreased, but totalSupply of afEth is still the same. As result, price of token decreases.

While this incorrect for everyone, this also can be used by attacker(some eth whale) in order to mint tokens cheaper. To do that he will need to deposit big amount of eth to the afEth. Then signal withdraw request for all his tokens and mint for example same amount. Soon, he will start earn profit every round he does so, as he will always mint tokens cheaper.

Tools Used

VsCode

Recommended Mitigation Steps

Change last line of price function: return ((vEthValueInEth + safEthValueInEth) * 1e18) / (totalSupply() - balanceOf(address(this)));

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

VotiumStrategyCore.applyRewards can be sandwhiched, so users rewards will be lost.

Proof of Concept

VotiumStrategyCore.applyRewards will be used in order to swap all rewards to eth and then distribute eth to the safEth or vEth.

The problem here is that sellCvx function doesn't have slippage protection, which means that attacker can sandwhich this function call and make contract lose funds during the swap. Pls, note, that AfEth.depositRewards also can't protect form this.

Tools Used

VsCode

Recommended Mitigation Steps

Deal with slippage when you call sellCvx function.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

VotiumStrategy.withdrawTime doesn't expect that balance can be already unlocked. As result user can wait more time to witdraw.

Proof of Concept

When user wants to withdraw, then he needs to initiate requestWithdraw. As some part of funds are locked as cvx token inside vlcvx that means that they should be withdrawn. When you lock cvx then amount is locked for some period, so you can't withdraw it for that time.

For such reason withdrawTime function exists, which purpose is to calculate time when needed amount will be available to withdraw.

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

    function withdrawTime(
        uint256 _amount
    ) external view virtual override returns (uint256) {
        uint256 _priceInCvx = cvxPerVotium();
        (
            ,
            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();
    }

unlockable is amount that can be withdrawn right now from VLCVX_ADDRESS. And totalLockedBalancePlusUnlockable is total available balance at the moment.

As you can see, this code doesn't check that totalLockedBalancePlusUnlockable > amount, it checks it only inside the loop. And because of that, it's possible that balance is already enough, but function will use time of first lockedBalances[0].unlockTime as withdraw time, which will make user wait more to withdraw.

Exactly same thing exists in the requestWithdraw function as it actually does almost same and tries to find epoch, when funds will be available.

Tools Used

VsCode

Recommended Mitigation Steps

Consider possibility, that totalLockedBalancePlusUnlockable > amount even without additional withdraw.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Last stakers may not receive funds back.

Proof of Concept

When user wants to withdraw, then he needs to initiate requestWithdraw. As some part of funds are locked as cvx token inside vlcvx that means that they should be withdrawn. When you lock cvx then amount is locked for some period, so you can't withdraw it for that time.

For such reason withdrawTime function exists, which purpose is to calculate time when needed amount will be available to withdraw.

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

    function withdrawTime(
        uint256 _amount
    ) external view virtual override returns (uint256) {
        uint256 _priceInCvx = cvxPerVotium();
        (
            ,
            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();
    }

This function reverts in case if it didn't trigger return in the loop. So protocol developers doesn't expect that it's possible.

However, it is. In case if there are no items in the lockedBalances, but unlockable != 0, that means that all locks are expired and such situation is likely to occur when users are going to quit AfEth token, so there is small amount of funds locked and new locks are not created.

So imagine situation that unlockable is 10 eth and user would like to withdraw it, but because loop will be skipped, then function will revert, which means that user can't withdraw. It will be still possible to recover funds by relocking, but user will have to wait more.

Same problem exists in the VotiumStrategy.requestWithdraw function.

Tools Used

VsCode

Recommended Mitigation Steps

As i recommended in another finding you need to expect that totalLockedBalancePlusUnlockable at the beginning is already enough to fulfill request.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

VotiumStrategyCore.applyRewards can be sandwhiched to make profit.

Proof of Concept

VotiumStrategyCore.applyRewards function will swap all rewards of contract into eth and then stake them into safEth or vEth contract. As result price of afEth token will increase.

Usually, when user want to withdraw, then he needs to make withdraw request, which will calculate when it will be possible to withdraw. So on that timestamp user will then call withdraw. So this is usually 2 step process that can't be done in same tx.

But it's also possible that on the moment of VotiumStrategyCore.applyRewards function call, unlockable amount will have some gap, that user can use to withdraw instantly.

In this case user can just deposit that gap amount right before, VotiumStrategyCore.applyRewards call and then request withdraw and withdraw, all in same block. This will be easy profit for use, but condition should satisfy: unlockable should have enough gap, so user can earn at least more than tx cost on this attack.

Tools Used

VsCode

Recommended Mitigation Steps

I don't know good solution.

Assessed type

Error

QA-01. VotiumStrategyCore.withdrawStuckTokens should not allow to withdraw cvx token.

Description

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L215-L220 VotiumStrategyCore.withdrawStuckTokens function can be used by malicious owner to withdraw cvx tokens from contract.

Recommendation

If token is cvx, then revert.

QA-02. AfEth.setRatio doesn't validate input.

Description

AfEth.setRatio doesn't validate input. Because of that it's possible that owner will provided incorrect ratio and contract will stop working.

Recommendation

Check that ratio is between 0 and e18.

G-01.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L160 Calculate vValue in such way: uint256 vValue = amount - sValue; and avoid additional arithmetical operations.

G-02.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L162-L164 Don't use += as totalValue is always 0. Change to:

totalValue =
            (sMinted * ISafEth(SAF_ETH_ADDRESS).approxPrice(true)) +
            (vMinted * vStrategy.price());