xETH - Versus Contest - d3e4's results

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/AMO2.sol#L249-L250 https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/AMO2.sol#L296-L297

Vulnerability details

Impact

A rebalance operation may overshoot, bringing the percentage outside the thresholds.

Proof of Concept

There are contractual limitations on the rebalance operations. It is assumed that these are put in place to ensure that the Rebalance Defender bot is not allowed to misbehave, such that trust can be placed in the contract rather than having to trust an off-chain bot.

The percentage of xETH is supposed to lie between REBALANCE_DOWN_THRESHOLD and REBALANCE_UP_THRESHOLD. AMO2.rebalanceUp() can only be called when the percentage of xETH is above REBALANCE_UP_THRESHOLD. This is supposed to burn xETH to bring the percentage back down to its proper range. It is possible that the amount burned is too great, such that the new percentage is below REBALANCE_DOWN_THRESHOLD. (There is a rebalanceUpCap which potentially restricts this, but there are no restrictions on how high this value can be, and it is statically set so it doesn't dynamically adjust to prevent this issue.). Once rebalanced, the next rebalancing can only happen afterCooldownPeriod.

The same is true, mutatis mutandis, for AMO2.rebalanceDown(), where xETH is minted to bring the percentage up.

If the percentage of xETH is too high, and if rebalanceUp() overshoots (burns too much), it may then be possible to undershoot (mint too much) such that the percentage is even higher than we started with.

Thus there is no contractual guarantee that rebalancing is properly executed.

Recommended Mitigation Steps

The ideal amount to burn/mint could be calculated in these functions themselves. A cheaper option is to simply check that the new percentage is within the thresholds. preRebalanceCheck() may be reused for this. In the latter option an even narrower constraint might be also used, for example by checking that the new percentage is within a certain distance of an ideal percentage. This ideal percentage may either be set independently (in between the thresholds) or calculated as, for example, the half-way point between then thresholds.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/wxETH.sol#L222-L256

Vulnerability details

Impact

If wxETH drips when nothing is staked, then the first staker can claim every drop.

Proof of Concept

Suppose drip is enabled when totalSupply() == 0. At least one block passes and the first staker stakes, just 1 xETH is enough. This mints her 1 wxETH. This also calls _accrueDrip() (by the drip modifier) which drips some amount of xETH. Note that _accrueDrip() is independent of totalSupply(), so it doesn't matter how little she stakes. cashMinusLocked is now 1 plus the amount dripped. Naturally, since she owns the entire supply she can immediately unstake the entire cashMinusLocked. Specifically, the exchangeRate() is now (cashMinusLocked * BASE_UNIT) / totalSupply() and she gets (totalSupply() * exchangeRate()) / BASE_UNIT = cashMinusLocked.

The issue is simply that drip is independent of staked amount, especially that it may drip even when nothing is staked, which enables the above attack.

Recommended Mitigation Steps

Note what happens when totalSupply() > 0. Then drops will fall on existing wxETH, and any new staker will trigger a drip before having to pay the new exchange rate which includes the extra drops. So a new staker in this case cannot unstake more than they staked; all drops go to previous holders. Therefore, do not drip when totalSupply == 0; in _accrueDrip():

-if (!dripEnabled) return;
+if (!dripEnabled || totalSupply() == 0) return;

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/wxETH.sol#L212

Vulnerability details

Impact

The first staker can inflate the exchange rate by transferring tokens directly to the contract such that subsequent stakers get minted zero wxETH. Their stake can then be unstaked by the first staker, together with their own first stake and inflation investment. Effectively, the first staker can steal the second stake. The attack exploits the rounding error in tokens minted, caused by the inflation. This vulnerability has a more general impact than just described, in that stakes might be partially stolen and that it also affects further stakers down the line, but the below example demonstrates the basic case.

Proof of Concept

Alice is the first staker, so totalSupply() == 0. She stakes 1 xETH by calling stake(1).

function stake(uint256 xETHAmount) external drip returns (uint256) {
    /// @dev calculate the amount of wxETH to mint
    uint256 mintAmount = previewStake(xETHAmount);

    /// @dev transfer xETH from the user to the contract
    xETH.safeTransferFrom(msg.sender, address(this), xETHAmount);

    /// @dev emit event
    emit Stake(msg.sender, xETHAmount, mintAmount);

    /// @dev mint the wxETH to the user
    _mint(msg.sender, mintAmount);

    return mintAmount;
}

and gets minted previewStake(1)

function previewStake(uint256 xETHAmount) public view returns (uint256) {
    /// @dev if xETHAmount is 0, revert.
    if (xETHAmount == 0) revert AmountZeroProvided();

    /// @dev calculate the amount of wxETH to mint before transfer
    return (xETHAmount * BASE_UNIT) / exchangeRate();
}

which thus is 1 * BASE_UNIT / exchangeRate(), where

function exchangeRate() public view returns (uint256) {
    /// @dev if there are no tokens minted, return the initial exchange rate
    uint256 _totalSupply = totalSupply();
    if (_totalSupply == 0) {
        return INITIAL_EXCHANGE_RATE;
    }

    /// @dev calculate the cash on hand by removing locked funds from the total xETH balance
    /// @notice this balanceOf call will include any lockedFunds,
    /// @notice as the locked funds are also in xETH
    uint256 cashMinusLocked = xETH.balanceOf(address(this)) - lockedFunds;

    /// @dev return the exchange rate by dividing the cash on hand by the total supply
    return (cashMinusLocked * BASE_UNIT) / _totalSupply;
}

so this works out to 1 * BASE_UNIT / INITIAL_EXCHANGE_RATE. This is what Alice is minted and also the new totalSupply().

Bob is about to stake b xETH (which Alice can frontrun).

Before Bob's stake, Alice transfers a xETH directly to the wxETH contract. The xETH balance of wxETH is now 1 + a + lockedFunds; 1 from Alice's stake, a from her token transfer and whatever lockedFunds may have been added.

Now Bob stakes his b xETH by calling stake(b). By reference to the above code, this mints him previewStake(b), which is n * BASE_UNIT / exchangeRate(). This time totalSupply() > 0 so exchangeRate() this time is (1 + a) * BASE_UNIT / (1 * BASE_UNIT / INITIAL_EXCHANGE_RATE), which simplifies to (1 + a) * INITIAL_EXCHANGE_RATE. So Bob gets minted b * BASE_UNIT / ((1 + a) * INITIAL_EXCHANGE_RATE). This may clearly be < 1 which is therefore rounded down to 0 in Solidity. BASE_UNIT and INITIAL_EXCHANGE_RATE are both set to 1e18, so Bob is minted b/(1 + a) tokens. In that case we simply have that if a >= b then Bob is minted 0 wxETH.

Now note in stake() above that whenever a non-zero amount is staked, those funds are transferred to the contract (even if nothing is minted).

Bob cannot unstake anything with 0 wxETH, so he has lost his b xETH.

totalSupply() is now 1 and the xETH balance of wxETH is 1 + a + b + lockedFunds. exchangeRate() is therefore (1 + a + b) * BASE_UNIT / 1.

Alice owns the 1 wxETH ever minted so if she unstakes it

function unstake(uint256 wxETHAmount) external drip returns (uint256) {
    /// @dev calculate the amount of xETH to return
    uint256 returnAmount = previewUnstake(wxETHAmount);

    /// @dev emit event
    emit Unstake(msg.sender, wxETHAmount, returnAmount);

    /// @dev burn the wxETH from the user
    _burn(msg.sender, wxETHAmount);

    /// @dev return the xETH back to user
    xETH.safeTransfer(msg.sender, returnAmount);

    /// @dev return the amount of xETH sent to user
    return returnAmount;
}

she gets previewUnstake(1)

function previewUnstake(uint256 wxETHAmount) public view returns (uint256) {
    /// @dev if wxETHAmount is 0, revert.
    if (wxETHAmount == 0) revert AmountZeroProvided();

    /// @dev calculate the amount of xETH to return
    return (wxETHAmount * exchangeRate()) / BASE_UNIT;
}

which thus is 1 * (1 + a + b) * BASE_UNIT / BASE_UNIT == (1 + a + b).

That is, Alice gets both hers and Bob's stakes.

Recommended Mitigation Steps

Do not use xETH.balanceOf(address(this)) when calculating the funds staked. Account only for funds transferred through stake() by keeping an internal accounting of the balance. Consider implementing a sweep function to access any unaccounted funds, or use them as locked funds if free (but unlikely) funds would be accepted as such.

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/wxETH.sol#L87

Vulnerability details

Impact

The drip might inflate the exchange rate on an initial stake such that that subsequent stakers get minted zero wxETH. Their stake can then be unstaked by the first staker, together with their own first stake and inflation investment. Effectively, the first staker might be able to steal the second stake.

Proof of Concept

The underlying principle is the same as the well-known inflation attack, where the exchange rate is inflated by a direct token transfer, which this contract is also vulnerable to. Refer to my separate report, titled "Inflation attack by token transfer ", on that issue for details, if needed. The difference here is that the inflation is instead caused by the drip.

Alice has the sole stake of 1 xETH, and owns the total supply of 1 wxETH (she can achieve this either by staking only this amount or by unstaking a large amount). The contract drips 1e18 - 1 xETH. The exchange rate is now (cashMinusLocked * BASE_UNIT) / _totalSupply = 1e18 * 1e18 / 1 = 1e36. When Bob next stakes b xETH, he will get (b * BASE_UNIT) / exchangeRate() = b / 1e18 wxETH. This suffers significant rounding losses; up to 1e18 - 1 xETH from b is rounded off. Especially, if b < 1e18 Bob gets nothing in return for his stake. These rounding losses are still staked, and therefore claimable by Alice when she unstakes.

Note that if b == 2e18 - 1, Bob will get 1 wxETH. The exchange rate will now be (3e18 - 1) * 1e18 / 2 = 1.5e36 - 0.5e18. When Carol stakes c xETH, she will get c * 1e18 / (1.5e36 - 0.5e18) wxETH. So if c < 1.5e18 she will, like Bob in the first case above, not get any wxETH for her xETH. So if Carol stakes 1.5e18 - 1 xETH, Alice and Bob can then each unstake their 1 wxETH and get about (3e18 - 1 + 1.5e18 - 1) / 2 = 2.25e18 - 1 xETH each. This is a profit of 2.25e18 - 2 xETH for Alice, and a 0.25e18 xETH profit for Bob. This demonstrates that the rounding losses persist and continue to benefit (primarily) Alice.

Suggested Mitigation Steps

Dripping is currently independent of staked amount. A cap may be put in place which restricts dripping which would make the exchange rate too high. The drip does not have to proportional to totalSupply() when it is large and rounding errors are not an issue, but if it were proportional at least at the low end, it would in a way be proportional to rounding losses, which should eliminate the issue. Rounding losses could be prohibited by only allowing stakes in amounts where no rounding loss occur. However, stakers would then still be forced to choose amounts in large discrete steps.

Assessed type

ERC4626

Precision loss

In AMO2.bestRebalanceUpQuote() and AMO2.bestRebalanceDownQuote() there are rounding losses of the form (a/b)*c <= a*c/b. Refactor

bestQuote.min_xETHReceived = applySlippage(
-     (vp * defenderQuote.lpBurn) / BASE_UNIT
- );
+     (vp * defenderQuote.lpBurn)
+ ) / BASE_UNIT;

and

bestQuote.minLpReceived = applySlippage(
-     (BASE_UNIT * defenderQuote.xETHAmount) / vp
- ); 
+     (BASE_UNIT * defenderQuote.xETHAmount) 
+ ) / vp;

No check that REBALANCE_UP_THRESHOLD >= REBALANCE_DOWN_THRESHOLD

It is assumed that REBALANCE_UP_THRESHOLD >= REBALANCE_DOWN_THRESHOLD (or even >), but when setting these values this is not guaranteed. Consider checking that this is fulfilled when setting them in AMO2.setRebalanceUpThreshold](https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/AMO2.sol#L495-L504) and [AMO2.setRebalanceUpThreshold`.

Redundant function

AMO2.addLiquidityOnlyStETH() can be removed if AMO2.addLiquidity() is modified

+ if (xETHAmount > 0)
xETH.mintShares(xETHAmount);

so that it may be called with xETHAmount == 0 without reverting. addLiquidityOnlyStETH(stETHAmount, minLpOut) can then be achieved by calling addLiquidity(stETHAmount, 0, minLpOut).

It's a percentage, not a ratio

What is called "ratio" in the comments in AMO2.preRebalanceCheck() is actually a percentage, xEthPct. Consider amending the comments to reflect this fact.

Remove commented-out code

This commented-out line is equivalent in its context to the line below. Remove it.

Redundant store

lastReport = block.number; in wxETH.stopDrip() is redundant. This is already done in the drip modifier. It is uneccesary to update lastReport in the constructor, since dripEnabled is false by default and has to be enabled by calling startDrip(), which immediately sets lastReport. For the same reason it is not necessary to update lastReport when stopping the drip. The next time it's started it will update. In general, only update when starting and dripping. The other functions don't care what lastReport was or will be.

Redundant function

AMO2.addLiquidityOnlyStETH() can be removed if AMO2.addLiquidity() is modified

+ if (xETHAmount > 0)
xETH.mintShares(xETHAmount);

so that it may be called with xETHAmount == 0 without reverting. addLiquidityOnlyStETH(stETHAmount, minLpOut) can then be achieved by calling addLiquidity(stETHAmount, 0, minLpOut).

isXETHToken0 can be a uint256

In the constructor of AMO2.sol we can refactor

constructor(
...
    - bool isXETHToken0
    + uint256 _xETHIndex
) {
...
    - xETHIndex = isXETHToken0 ? 0 : 1;
    - stETHIndex = isXETHToken0 ? 1 : 0;
    + xETHIndex = _xETHIndex;
    + stETHIndex = 1 - _xETHIndex;

This reverts if _xETHIndex isn't 0 or 1, and is cheaper.