Loss of funds on deposit when `totalShares > 0 && priorTokenBalance == 0`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/strategies/StrategyBase.sol#L78-L156

Vulnerability details

Impact

Withdrawing one's shares may return far less tokens than one deposited.

Proof of Concept

Shares returned when depositing is calculated in StrategyBase.deposit() as

if (totalShares == 0) {
        newShares = amount;
    } else {
        uint256 priorTokenBalance = _tokenBalance() - amount;
        if (priorTokenBalance == 0) {
            newShares = amount;
        } else {
            newShares = (amount * totalShares) / priorTokenBalance;
        }
    }

Tokens returned when withdrawing is calculated in StrategyBase.withdraw() as

if (priorTotalShares == amountShares) {
    amountToSend = _tokenBalance();
} else {
    amountToSend = (_tokenBalance() * amountShares) / priorTotalShares;
}

(Note that the priorTotalShares == amountShares branch is unnecessary.) The only way we can have a zero supply of shares but non-zero token balance is if tokens are sent directly to the contract. These can then simply be swept up by depositing tokens, receiving the only existing shares and then withdrawing all shares to get the entire token balance. So this is not an issue. Essentially, whenever the supply of shares goes to zero, everything is reset. When both the supply of shares and token balance are non-zero, say S and T respectively, if we deposit a tokens we get a * S / T shares and the share supply is now S + a * S / T and the token balance is T + a. If we then withdraw our a * S / T shares we get (T + a) * (a * S / T) / (S + a * S / T) = (T + a) * a / (T + a) = a tokens back (not counting rounding losses). A similar calculation shows that first withdrawing s shares and then depositing the received tokens back we also get s shares back. Just as intended.

Now consider the case where the token balance drops to zero, while there are still S shares. Then the shares are worthless, and withdrawing them sends no tokens in return, which makes sense. For deposits a token balance of zero would imply division by zero, so this case is handled by if (priorTokenBalance == 0) newShares = amount;. However, now a deposit of a tokens returns a shares and sets the share supply to S + a and the token balance to a, but withdrawing these a shares only sends back a * a / (S + a) tokens back, which is clearly less than a. Effectively, the depositor has been forced to share his deposit with all other existing shares.

Recommended Mitigation Steps

Note the difference if the balance only drops to 1. Then a deposit of a tokens returns a*S shares and sets the share supply to S + a*S and the token balance to a, and withdrawing these a*S shares sends back a * a*S / (S + a*S) = a * a / (1 + a) = a - 1 tokens back. A solution is therefore to not "allow" the balance to drop to zero, by pretending that it is at least 1 and returning amount * totalShares shares in the case priorTokenBalance == 0.

Assessed type

ERC4626

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

Sidu28 marked the issue as sponsor disputed

[Sidu28]

We cannot simply 'consider the case where the token balance drops to zero'. This is a hypothetical complete draining of the contract's token balance with no justification.

[GalloDaSballo]

By definition, the only way to re-base the shares downward is either a loss of funds or a single owner withdrawing all funds

That scenario invalidates the idea of another depositor getting their shares negatively rebased

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

[d3e4]

The strategy may manage the funds actively. From the docs: "Stakers who wish to deposit ERC20 tokens can do so by calling the StrategyManager, which will transfer the depositor’s tokens to a user-specified Strategy contract, which in turn manages the tokens to generate rewards in the deposited token (or just passively holds them, if the depositor is risk-averse or if the token lacks good reward-generating opportunities)."

"the strategy may either actively or passively manage the funds"

The strategy may thus be expected to move the tokens out of the contract. Hopefully the inheriting strategy will override the _tokenBalance() to account for this, but the balance may still drop to zero in the case it turns a loss. So shares of a zero balance is possible.

[GalloDaSballo]

The additional hypotheticals:

• Strategy moves funds out
• Strategy moves all funds
• Strategy report a 0 balance

Now that I think of it, if the Strategy reported a 0 balance due to a loss, then the loss would have happened because of the Strategy and not because of the 0 value causing a reset

0 value resets are valid attack vectors (see Yearn Vaults in Lending Protocols, see Comp tokens resetting), however at this time. with not specific Strategy allowing a loss, I don't think the finding has enough evidence for it to be valid.

I would recommend the Warden to check once a Strategy Implementation is known and to follow up with the Sponsor at that time

[d3e4]

0 value resets are valid attack vectors (see Yearn Vaults in Lending Protocols, see Comp tokens resetting), however at this time. with not specific Strategy allowing a loss, I don't think the finding has enough evidence for it to be valid.

I would recommend the Warden to check once a Strategy Implementation is known and to follow up with the Sponsor at that time

No strategy at all exists at this time. Isn't the point to protect the future strategies inheriting from StrategyBase? As explained above the description of the expected strategies imply the possibility of strategies which would be susceptible to this, since they should inherit from StrategyBase which contains this bug.

[d3e4]

0 value resets are valid attack vectors (see Yearn Vaults in Lending Protocols, see Comp tokens resetting), however at this time. with not specific Strategy allowing a loss, I don't think the finding has enough evidence for it to be valid. I would recommend the Warden to check once a Strategy Implementation is known and to follow up with the Sponsor at that time

No strategy at all exists at this time. Isn't the point to protect the future strategies inheriting from StrategyBase? As explained above the description of the expected strategies imply the possibility of strategies which would be susceptible to this, since they should inherit from StrategyBase which contains this bug.

@GalloDaSballo

[GalloDaSballo]

There is no rational scenario in which shares would be outstanding and the balance would be exactly 0

You can research this once an implementation is known and get a valid award