Inflation attack

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

AfEth.price() can be inflated, causing severe rounding errors or constraints for subsequent deposits.

Proof of Concept

(1) An initial depositor can deposit() ETH for an arbitrarily small amount of afEth, e.g. 1 afEth.

(2) Both safEth and vAfEth can be staked and deposited into directly, so by obtaining these directly by himself outside of an AfEth deposit he can then

(3) inflate the price() by transferring a large amount of safEth and/or vAfEth (Votium strategy tokens), e.g. 1e18 Wei's worth, directly to the AfEth contract.

price() is calculated as the value in ETH of AfEth's balances of safEth and vAfEth per total supply of afEth; in this example 1e18 Wei/afEth. That is, the next depositor will receive only 1 afEth per whole ETH deposited, any remaining fraction being lost to rounding.

deposit() has a parameter _minout which protects the next depositor against the usual frontrunning inflation attack where the victim unexpectedly suffers a rounding loss. But once the inflation attack has been performed the subsequent depositors will either suffer severe rounding losses or be constrained to deposit in highly granular amounts. Effectively, afEth will have less decimals (or none or negative) and be practically useless.

Recommended Mitigation Steps

Any of the above (1) - (3) has to remedied. (2) may be a desirable feature and should probably remain.
(1) is remedied by making sure that the total supply be either 0 or greater than some sufficiently large amount (e.g. 1e9). One possibility is by simply making an initial deposit oneself.
(3) can be remedied by accounting only the safEth and vAfEth obtained by the contract via the proper pathway of deposit(). This renders any direct transfers to the contract invisible to it.

Assessed type

ERC4626

[0xleastwood]

I'm not sure if I follow as to how this is an inflation attack? We increase the price of each afEth token which means depositors mint less tokens? But those tokens are redeemable for the same amount regardless of whether price() has been inflated or not.

I also understand what you mean by front-running user deposits, but vEthStrategy.balanceOf() is not used in any division in regards to price(), so this is not achievable either.

[c4-judge]

0xleastwood marked the issue as primary issue

[0xleastwood]

#35 refers to the correct instance of such an inflation attack. I'm not sure if this is consistent with that in anyway.

[c4-judge]

0xleastwood marked the issue as unsatisfactory:
Invalid

[d3e4]

I'm not sure if I follow as to how this is an inflation attack? We increase the price of each afEth token which means depositors mint less tokens? But those tokens are redeemable for the same amount regardless of whether price() has been inflated or not.

Maybe some clarification of what the inflation attack is fundamentally about is warranted. The so-called inflation attack is usually conceived of as stealing the entire first deposit by front-running with a tiny deposit and balance donation. That is just one way to exploit this vulnerability, but the essence of the inflation attack is an inflation of the rounding errors. The usual exploit is the rounding of <1 to 0, which implies a 100 % loss of the deposit. But rounding e.g. <5 down to 4 implies a loss of 20 %, which is still significant.

I also understand what you mean by front-running user deposits, but vEthStrategy.balanceOf() is not used in any division in regards to price(), so this is not achievable either.

It is in the numerator in price(), and price() is in the denominator in the mint amount calculation. So vEthStrategy.balanceOf() is indeed in the denominator of the minted amount.

#35 refers to the correct instance of such an inflation attack. I'm not sure if this is consistent with that in anyway.

#35 is about an inflation attack in VotiumStrategy. This is only an issue insofar it affects AfEth. No guarantees are made with regards to interacting with VotiumStrategy directly; depositing there might as well be like sending funds to the zero-address. As for how much afEth is minted it doesn't matter whether we donate to AfEth's votium balance, or VotiumStrategy's CVX balance; both balances are factors in the vEthValueInEth used in the mint calculation.
Thus #58 describes the general case for inflation, whereas #35 describes the specific pathway of inflating in VotiumStrategy. However, #35 fails to mention how the inflation is an issue for AfEth despite the _minOut parameter. The impact according to #35 only pertains to users interacting directly with VotiumStrategy, which no one has said is safe to do.

Let's say we mitigate against inflation by making a pre-deposit. Then mitigating #35, i.e. pre-depositing into VotiumStrategy, does not mitigate #58, but pre-depositing into AfEth mitigates both.

[0xleastwood]

I understand where you are coming from but I think there is a lot of context missing in this issue. The attacker incurs rounding on their mint amount but those fewer minted tokens are still worth more tokens from the perspective of the user so is there really any loss? The attacker ultimately loses a significant amount by transferring into the contract.

[d3e4]

@0xleastwood Yes, there is a loss. The price is irrelevant; this is a rounding error issue.
The minting and withdrawal arithmetic is only correct in rational numbers. Solidity always rounds down a theoretical rational to an integer. To mitigate this numerators are typically multiplied by 1e18. So if the price is 1.1e18 a deposit of 0.5e18 ETH should theoretically mint 0.5e18 * 1e18 / 1.1e18 = 454,545,454,545,454,545 + 5/11 shares, but which in Solidity is rounded down to 454_545_454_545_454_545 shares. This therefore implies a loss of 5/11 shares, which of course is a negligible number in comparison to 454_545_454_545_454_545.
But if the price is inflated to 1.1e35 then the same deposit should mint 0.5e18 * 1e18 / 1.1e35 = 4 + 5/11 shares, which in Solidity is rounded down to 4 shares. This implies a loss of 5/11 which is a considerable number in comparison to 4. It implies a 12% loss.

Any rounding loss must go somewhere, so it goes to the inflator, since he inflated the price after he got his shares.

What would the user put as _minOut in this case? He cannot put 4 + 5/11 because Solidity doesn't support it. He cannot put 5, because he is not even supposed to get that much. So the remaining option is to put 4, but that implies a loss of 12%. The only thing he could do (if he is aware of this) is to instead deposit only 0.44e18 ETH for which is minted 0.44e18 * 1e18 / 1.1e35 = 4 shares, where no rounding is necessary. If he instead want to be minted 5 he must provide exactly 0.55e18 ETH. Thus the users are forced to deposit in multiples of 0.11e18 ETH.

_minOut doesn't protect against the generalised inflation attack at all; only against a very specific attack.

#35 and #26 describe inflation of the VotiumStrategy price. The immediate impact of this is loss for those who deposit there. This impact is not much of an issue, since users are not supposed to deposit there. This is the same reason why you judged #33 as QA.
However, the inflation of VotiumStrategy extends into AfEth and this is where it matters.
Strictly speaking #35 and #26 do not mention this ultimate impact. And #58 only mentions inflation in AfEth. This can be taken to mean that they are two separate issues, with similar impacts, where the impact only can be inferred from #35 and #26 while it is explicitly stated in #58.

[0xleastwood]

I think this is extremely hypothetical and you are cherrypicking numbers to support your case. But I do understand the issue with rounding and I think donation style attacks should always be avoided even if not directly exploitable. Moving to QA.

[c4-judge]

0xleastwood removed the grade

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[0xleastwood]

Also, you incorrectly assume the inflator benefits fully, unless they have complete control of the token supply before new deposits into the strategy, the rounded tokens are distributed to all existing token holders. So you're detailing an economic attack which requires significant capital (likely not even feasibly possible considering the total supply of ethereum) and an incorrectly configured _minOut which would normally identify such an attack. I do think a seed deposit is wise to say the least, but this issue is based entirely on hypotheticals.

[d3e4]

I think this is extremely hypothetical

This is not more hypothetical than #35. The attack here involves, just like #35, an initial deposit, immediately followed by a donation of assets. They are based on exactly the same assumptions.

you are cherrypicking numbers to support your case

Not at all. The attacker is in complete control of how much to inflate the price. If executed as a front-running attack he can match the victims _minOut and skim off any excess. In the standard example _minOut == 0 (because it doesn't exist), so the attacker can skim off the entire amount.
_minOut protects against one thing only: stealing all of the victim's deposit (simply because he wouldn't (but might!) put _minOut == 0).
However, I consider the more severe impact the potential to reduce the decimals of afEth, which renders the contract all but useless.

Also, you incorrectly assume the inflator benefits fully, unless they have complete control of the token supply before new deposits into the strategy, the rounded tokens are distributed to all existing token holders.

Again, he does have complete control of the supply since he it the first depositor, just like he must be in #35.

[0xleastwood]

Again, you are still cherrypicking numbers. Convex has a total token supply of 100_000_000 which can be represented as 1e26 with 18 decimals precision. The amount of rounding you would realistically get with this is minimal as compared to the cost of accruing such capital, and what does the attacker gain? A small amount of eth that has been rounded off?

[0xleastwood]

Basically the amount of rounding would be in the order of 1e10 which is nothing.

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood marked the issue as duplicate of #35

[0xleastwood]

I did not mean to upgrade this, it looks like it was previously linked to #35.

[c4-judge]

0xleastwood marked the issue as not a duplicate

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[d3e4]

Again, you are still cherrypicking numbers. Convex has a total token supply of 100_000_000 which can be represented as 1e26 with 18 decimals precision. The amount of rounding you would realistically get with this is minimal as compared to the cost of accruing such capital, and what does the attacker gain? A small amount of eth that has been rounded off?

The supply and decimals of Convex are irrelevant. The inflation and rounding is in AfEth.

The AfEth supply is initially 0.
The attacker deposits 1 wei for 1 AfEth. AfEth supply is now 1, the value of underlying is 1. Price is 1/1 = 1.
The attacker then donates 1e18 wei worth of underlying to AfEth. The supply of AfEth is still 1, but the value of underlying is 1e18 + 1. Price is 1/(1e18 + 1) ≈ 1e-18.

It's just a standard inflation attack. Maybe you are confused by focussing on #35 speaking about VotiumStrategy and a missing _minOut. The inflation attack is also possible in VotiumStrategy. But _minOut is NOT a protection against inflation.

[0xleastwood]

The supply and decimals of Convex are irrelevant. The inflation and rounding is in AfEth.

The AfEth supply is initially 0. The attacker deposits 1 wei for 1 AfEth. AfEth supply is now 1, the value of underlying is 1. Price is 1/1 = 1. The attacker then donates 1e18 wei worth of underlying to AfEth. The supply of AfEth is still 1, but the value of underlying is 1e18 + 1. Price is 1/(1e18 + 1) ≈ 1e-18.

It's just a standard inflation attack. Maybe you are confused by focussing on #35 speaking about VotiumStrategy and a missing _minOut. The inflation attack is also possible in VotiumStrategy. But _minOut is NOT a protection against inflation.

Now you are just moving the goal post to support your case. Post-judging QA is for adding additional context and this was clearly not outlined or even mentioned in your original issue. I don't believe this was the intention of the original issue and I also believe _minOut to be effective in identifying such an attack too.

[0xleastwood]

This issue will remain as QA.

[d3e4]

this was clearly not outlined or even mentioned in your original issue

What do you mean? This is exactly what I wrote in my Proof of Concept.

I also believe _minOut to be effective in identifying such an attack too

Which of the attacks? _minOut does nothing to prevent the increased granularity, and little to prevent skimming.
But whatever effectiveness that comes from the _minOut in AfEth has the same effect on VotiumStrategy. And it is only the effect on AfEth that is fully impactful, since users are not meant to deposit directly into VotiumStrategy, for which same reason you downgraded #33.

[c4-sponsor]

elmutt (sponsor) confirmed

[0xleastwood]

So we agree it is not similar to #35 in the sense that you can't perform the same front-running deposit attack because _minOut protects against this. However, _minOut would also effectively protect against significant rounding would it not? The expected amount of tokens to be minted can be computed prior to calling the deposit function and therefore _minOut can be configured accordingly. You assume that there is some skimming which would only be available on the first deposit let's say, but it is also assumed that _minOut is not configured properly.

I just can't see how this attack would be possible and I think you are creating very specific scenarios to support your case that aren't necessarily true.

I need you to stop comparing this to other issues because I have already stated my reasons for this in #35 and #33.

[d3e4]

#58 and #35 have the same impact (inflation of AfEth price) by different means. #35 also has another impact (inflation of VotiumStrategy price) which per #33 is not H/M.

_minOut has no effect on rounding. What it does is it gives the user a way to opt out of unpredictable mint amounts (such as due to market fluctuation). If there is significant rounding present, this is NOT unpredictable, but exactly known in advance by checking the intrinsic price (which one would have to do to even begin considering _minOut). In this situation the user achieves nothing by using _minOut. It is like saying: "for depositing 0.5e18 ETH I should get about 4.55 AfEth, but I know that this will be rounded down to 4, so I am going to put _minOut = 5 so that this doesn't happen". Then why even call the function?

[elmutt]

we believe this will solve it:

asymmetryfinance/afeth#179