`price()` in `AfEth.sol` doesn't take afEth held for pending withdrawals into account

[c4-submissions]

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

[elmutt]

@toshiSat I think we cansolve this by burning the tokens in requestWithdraw

[elmutt]

https://github.com/asymmetryfinance/afeth/pull/162

[c4-judge]

0xleastwood marked the issue as duplicate of #59

[c4-judge]

0xleastwood marked the issue as not a duplicate

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed