Rounding loss inflation

[code423n4]

Lines of code

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

Vulnerability details

Impact

Inflation attack is inadequately mitigated. First depositor can inflate the balance for profit.

Proof of Concept

The inflation attack is well-known. See this for an overview.
While StrategyBase.sol purports to implement a mitigation against inflation attacks, it is exhibiting a variant of the partial mitigation described in the above link under Example 2, i.e. that the first depositor cannot inflate the balance so much that the second depositor gets zero shares. This is required at L104.
There is an additional mitigation in this contract: the first depositor must also deposit, and get as shares, at least 1e9. This is enforced at L106 and L139.
But this just means that the first depositor would have to invest 1e9 times more than in the case where she could deposit just 1 token. This at least strongly reduces the incentive for the attack as it is specifically outlined in the link above.

However, the inflation attack is not limited to just robbing the second deposit. The root issue is that the shares to issue on deposit are rounded down by up to 1 share. This means that up to one share's worth of tokens are deposited in vain and not issued a share, but just transferred to the contract, which then benefit all shares equally.
Usually this is not a problem since 1 share is worth about 1 token, so the loss is negligible. But if the balance is inflated the value of each share in tokens may become significant. In this case the inflated value of the shares is at least 1e9 times less than the amount to which the balance is inflated.

For example, if an attacker deposits the minimum 1e9 tokens, and then transfers 1000e18 - 1e9 tokens to the contract, then the shares are worth 1e12 tokens per share. This means that each deposit made after the attacker may incur a rounding loss of up to 1e12 - 1 tokens. We can expect half of this on average. If the attacker holds half of the shares she will therefore make on average 0.25e12 tokens per deposit made. It is expected that many deposits (and withdrawals) will be made, so the attacker's profit is unbounded.

Recommended Mitigation Steps

Since deposits and withdrawals are strictly handled by the StrategyManager, it seems contrary to design to ever intend tokens to be transferred directly to the strategy. Therefore, keeping an internal accounting of the balance seems an appropriate solution. In this case the minimum share amount of 1e9 is no longer needed.

Another mitigation is to minimize the rounding loss. We can require that amount is optimal, by checking, in StrategyManager._depositIntoStrategy(), that reducing amount by 1 also reduces the number of shares:

// deposit the assets into the specified strategy and get the equivalent amount of shares in that strategy
+ uint256 lessShares = strategy.underlyingToSharesView(amount - 1);
shares = strategy.deposit(token, amount);
+ require(lessShares < shares, "amount can be reduced to limit rounding losses");

This allows the amount to deposit to be reduced off-chain, which saves gas. Otherwise, it may of course also be reduced on-chain.
Since shares = (amount * totalShares) / priorTokenBalance, we can reduce amount to amount -= ((amount * totalShares) % priorTokenBalance) / totalShares. We see that the amount can be reduced by up to priorTokenBalance/totalShares and the loss is now at worst 1 token.
Note that this solution still requires a mitigation like the already implemented minimum shares of 1e9. Otherwise the possible amounts to deposit may increment in steps too great after inflation, which would expose a kind of griefing risk.

Assessed type

ERC4626

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #193

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as duplicate of #343

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)