Unsafe use of `balanceOf(address(this))`

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/6b4867491350f8327d0ac4f496f263642cf3c1be/contracts/AfEth.sol#L183-L185

Vulnerability details

Impact

AfEth.deposit() can be bricked.

Proof of Concept

AfEth makes use of its own balance of afEth as a temporary store of afEth for withdrawal requests. On requestWithdraw() afEth is transferred to the AfEth contract and these are then burned on withdraw(). The contract's balance of afEth is excluded from the calculation of the withdrawRatio. One can thus cause the contract to withdraw all its underlying with less than its total supply. This implies a price of zero by which is divided in deposit().

1. deposit() some ETH for 2 afEth.
2. Transfer 1 afEth to the AfEth contract.
3. requestWithdraw() the remaining 1 afEth. This is precisely totalSupply() - afEthBalance in

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

   so withdrawRatio is set to 100 % and the entire safEth and vAfEth balances are withdrawn. The request may or may not be finalised by withdraw(); the withdrawals are still implicit in the accounting. Only the 1 afEth transferred with the withdrawal request would be burned; the totalSupply() still carries the 1 afEth transferred directly to the contract.

Now price()

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();
}

returns 0.

This causes deposit() to revert at uint256 amountToMint = totalValue / priceBeforeDeposit;.

deposit() is the only intended way to bring the safEth and vAfEth balances to non-zero so the only way out would be to send safEth or vAfEth directly to the contract. But all this would of course be claimable by the very next deposit, and so we return to the same state. The contract is effectively bricked.

Recommended Mitigation Steps

The afEth transferred by requestWithdraw() could be specifically accounted instead of just using the contract balance.

But a better solution is to just burn the afEth immediately in requestWithdraw(). Since requestWithdraw() immediately fixes the withdrawal in terms of safEth and CVX, it doesn't matter for the withdrawal when the afEth is burned.

Assessed type

ERC4626

[elmutt]

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

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as duplicate of #25

[0xleastwood]

The fix and impact is technically the same as #25 as we are incorrectly calculating the circulating supply of tokens which is used to calculate price().

[c4-judge]

0xleastwood marked the issue as satisfactory