Price inflation by locking CVX on behalf of VotiumStrategy

[c4-submissions]

Lines of code

https://github.com/asymmetryfinance/afeth/blob/74f340568480aa03d043e970fcf2578bea037cf6/contracts/strategies/votium/VotiumStrategyCore.sol#L148

Vulnerability details

Impact

The price of vAfEth can be inflated with severe rounding errors as a result.

Proof of Concept

In VotiumStrategy the price of vAfEth is calculated by

function cvxInSystem() public view returns (uint256) {
    uint256 total = ILockedCvx(VLCVX_ADDRESS).lockedBalanceOf(
        address(this)
    );
    return total + trackedCvxBalance;
}

function cvxPerVotium() public view returns (uint256) {
    uint256 supply = totalSupply();
    uint256 totalCvx = cvxInSystem() - cvxUnlockObligations;
    if (supply == 0 || totalCvx == 0) return 1e18;
    return (totalCvx * 1e18) / supply;
}

function price(bool _validate) external view override returns (uint256) {
    return (cvxPerVotium() * ethPerCvx(_validate)) / 1e18;
}

Making an initial deposit so that supply == 1 and then making total very large will thus inflate the price. total is the amount locked in the CVX locker for VotiumStrategy. There is nothing that prevents an attacker from calling ILockedCvx(VLCVX_ADDRESS).lock(votiumStrategyAddress, ...) to lock funds on behalf of VotiumStrategy. This is thus equivalent to donating underlying without increasing supply.

The price inflation in VotiumStrategy is felt by AfEth, which is where subsequent deposits would be made. The full standard attack would be to deposit in AfEth for 1 CVX locked (and around 1 safEth and 2 afEth minted), then lock e.g. 2e18 CVX on behalf of VotiumStrategy. The price of afEth would now be about 1e18, which causes rounding errors of up to about 1e18.

Recommended mitigation steps

A possibility is to mitigate this just like M-08: Inflation attack in VotiumStrategy, i.e. by keeping an internal accounting of locked balances. But I think the best would be to just make an initial deposit oneself on deployment. 1e9 wei should be sufficient. This prevents all kinds of inflation attacks.

Assessed type

Other

[romeroadrian]

The inflation attack is a way of stealing the initial deposit, not decreasing minting precision. You already submitted this here https://github.com/code-423n4/2023-09-asymmetry-findings/issues/58 and judged as QA since there is no benefit to the attacker and the vault can be simply redeployed.

[c4-sponsor]

toshiSat marked the issue as disagree with severity

[c4-sponsor]

toshiSat (sponsor) acknowledged

[toshiSat]

The plan was always to mint and lock afEth tokens on launch

[d3e4]

The inflation attack is a way of stealing the initial deposit, not decreasing minting precision. You already submitted this here code-423n4/2023-09-asymmetry-findings#58 and judged as QA since there is no benefit to the attacker and the vault can be simply redeployed.

The initial deposit is stolen by means of decreasing the minting precision.

The linked issue is a completely different mechanism.

[romeroadrian]

The inflation attack is a way of stealing the initial deposit, not decreasing minting precision. You already submitted this here code-423n4/2023-09-asymmetry-findings#58 and judged as QA since there is no benefit to the attacker and the vault can be simply redeployed.

The initial deposit is stolen by means of decreasing the minting precision.

The linked issue is a completely different mechanism.

Right it's a mechanism for decreasing the precision, not for stealing deposits. Here you are just griefing the minting precision with no impact.

[d3e4]

Right it's a mechanism for decreasing the precision, not for stealing deposits. Here you are just griefing the minting precision with no impact.

This is not a griefing. The decrease in precision is precisely what causes the loss of funds. It can be used, for example, to steal the initial deposit. The impact is a loss of funds.

[d3e4]

Sorry, my comments addressed your claims regarding inflation in general. You are right that with proper usage of _minout the possibility of theft of funds is in this case less likely and that this is mostly a griefing attack on the protocol functionality. This should thus have been graded as Medium rather than High.

[0xleastwood]

This was already highlighted previously, it will not be rewarded again

[c4-judge]

0xleastwood marked the issue as nullified

[d3e4]

@0xleastwood, I'm note sure where we misunderstand each other, but let me give a few example scenarios enabled by this issue:

1. The first user naively deposits with a _minout == 0 (because why bother when the (sans inflation) price() is known to start at 1e18?). The full normal inflation attack is possible and the attacker steals his entire deposit.
2. The first user wants to deposit 1 ETH and sets _minout to 95% of the price() (how else would he determine _minout?). The attacker can skim off the 5%.
3. The attacker inflates the price so that the rounding errors are the equivalent of $100. The first depositors experience little problem with this because they deposit large amounts in multiples of $100, so their rounding losses are only a few percent and don't revert. The attacker has perhaps already made a small profit now. However, now the protocol is established and user experience for smaller fish will be unbearable having to deposit in such large multiples or suffering rounding losses up to $100. The protocol loses reputation and may not be able to regain it after redeploying a new uninflated one.

Do you agree that any of these impacts warrants a HM grade? Do you agree that any of these scenarios are possible?

[0xleastwood]

@0xleastwood, I'm note sure where we misunderstand each other, but let me give a few example scenarios enabled by this issue:

1. The first user naively deposits with a _minout == 0 (because why bother when the (sans inflation) price() is known to start at 1e18?). The full normal inflation attack is possible and the attacker steals his entire deposit.
2. The first user wants to deposit 1 ETH and sets _minout to 95% of the price() (how else would he determine _minout?). The attacker can skim off the 5%.
3. The attacker inflates the price so that the rounding errors are the equivalent of $100. The first depositors experience little problem with this because they deposit large amounts in multiples of $100, so their rounding losses are only a few percent and don't revert. The attacker has perhaps already made a small profit now. However, now the protocol is established and user experience for smaller fish will be unbearable having to deposit in such large multiples or suffering rounding losses up to $100. The protocol loses reputation and may not be able to regain it after redeploying a new uninflated one.

Do you agree that any of these impacts warrants a HM grade? Do you agree that any of these scenarios are possible?

Regarding your points, 1 and 2 are assuming improper use of _minout. I think this is expected and similar to how uniswap allows users to configure their own slippage. It is their responsibility to do this correctly.

In terms of your 3rd point, I'm not sure how the attacker benefits from rounding, I think in this setup they would actually have the funds they locked on behalf of the contract diluted as other users mint tokens.

[0xleastwood]

Basically, a sufficiently capitalised user could sidestep the attacker and basically steal funds that they had locked on behalf of the votium contract.

[d3e4]

Regarding your points, 1 and 2 are assuming improper use of _minout. I think this is expected and similar to how uniswap allows users to configure their own slippage. It is their responsibility to do this correctly.

I would have preferred to protect against MEV-attacks and not put users at risk when avoidable, but fair enough.

In terms of your 3rd point, I'm not sure how the attacker benefits from rounding, I think in this setup they would actually have the funds they locked on behalf of the contract diluted as other users mint tokens.

All rounding losses end up in the vault and are thus shared by all shares equally. The attacker did not suffer any rounding loss on deposit, so he only stands to profit. He can always reclaim at least what he deposited and donated.

Basically, a sufficiently capitalised user could sidestep the attacker and basically steal funds that they had locked on behalf of the votium contract.

He can only steal from the profit, not from what the attacker spent. A whale could deposit without rounding loss and thereby claim a large portion of the rounding losses in the vault.

The main point of 3. is not necessarily the profit, but damaging the protocol. Would you not agree that the scenario laid out in 3. is problematic for the protocol?

[0xleastwood]

I think this becomes extremely tricky to perform because of the use of trackedvStrategyBalance in AfEth.price() is it not? I guess this shouldn't be problematic if we deposit a single share. But (vEthStrategy.price(_validate) * trackedvStrategyBalance) / 1e18 would round things down significantly.

[0xleastwood]

I think the extent of rounding is being overstated here. Let's say we deposit 1 share here and inflate the price of this share to be something like 1000 ETH, then vEthValueInEth is calculated as 1000e18 * 1 / 1e18 = 1000 and then scaled to 1000e18 later in the price() function.

The rounding happens in uint256 amountToMint = totalValue / priceBeforeDeposit of AfEth.deposit(). To avoid rounding down and truncating, a user would need to deposit totalValue greater than 1000e18. So this seems problematic.

Some things that may make this difficult to pull off:

• vStrategy.deposit() will not allow you to purchase 1 wei of convex so this attack becomes even more expensive. Hard to say if you could even purchase such a small amount.

But, because balances are being tracked and used within withdrawals, the attacker is unable to reclaim their deposited amount used to pull this off. So I think this is not even possible in the first place as the attacker is effectively burning their capital to increase rounding in the protocol.

For example, requestWithdraw() has a parameter _amount which is what is "burnt". As the attacker only minted 1 wei of shares, this is the maximum amount they are able to burn. withdrawRatio is equal to 1e18 in this case and we get:

uint256 votiumWithdrawAmount = (withdrawRatio *
            trackedvStrategyBalance) / 1e18
...
uint256 safEthWithdrawAmount = (withdrawRatio * safEthBalance) / 1e18;

withdrawRatio is strictly less than 1e18 and trackedvStrategyBalance is 1 wei under best attack conditions. So both these amounts round to zero and the attacker receives nothing. You would need to carefully craft what amount of shares are initially minted to avoid significant rounding in requestWithdraw().

[d3e4]

I think this becomes extremely tricky to perform because of the use of trackedvStrategyBalance in AfEth.price() is it not? I guess this shouldn't be problematic if we deposit a single share. But (vEthStrategy.price(_validate) * trackedvStrategyBalance) / 1e18 would round things down significantly.

The inflation is done on total, which is not tracked.

I think the extent of rounding is being overstated here. Let's say we deposit 1 share here and inflate the price of this share to be something like 1000 ETH, then vEthValueInEth is calculated as 1000e18 * 1 / 1e18 = 1000 and then scaled to 1000e18 later in the price() function.

The rounding happens in uint256 amountToMint = totalValue / priceBeforeDeposit of AfEth.deposit(). To avoid rounding down and truncating, a user would need to deposit totalValue greater than 1000e18. So this seems problematic.

Is this not the same extent of rounding as in the normal inflation attack? So how is it overstated?

• vStrategy.deposit() will not allow you to purchase 1 wei of convex so this attack becomes even more expensive. Hard to say if you could even purchase such a small amount.

Because 1 wei ETH is about 526 wei CVX? This is just an exchange rate. You then inflate with 526e18 CVX, which is 1e18ETH, to inflate the ETH price by 1e18. This is neither more nor less expensive.

But, because balances are being tracked and used within withdrawals, the attacker is unable to reclaim their deposited amount used to pull this off. So I think this is not even possible in the first place as the attacker is effectively burning their capital to increase rounding in the protocol.

The inflated variable, total, is not tracked. But even if it were true that the attacker just burned their capital, it would still be a griefing attack (note that my griefing example only involved $100, not 1000 ETH).

For example, requestWithdraw() has a parameter _amount which is what is "burnt". As the attacker only minted 1 wei of shares, this is the maximum amount they are able to burn. withdrawRatio is equal to 1e18 in this case and we get:

uint256 votiumWithdrawAmount = (withdrawRatio *
            trackedvStrategyBalance) / 1e18
...
uint256 safEthWithdrawAmount = (withdrawRatio * safEthBalance) / 1e18;

Indeed, withdrawRatio is equal to 1e18,

withdrawRatio is strictly less than 1e18 and trackedvStrategyBalance is 1 wei under best attack conditions. So both these amounts round to zero and the attacker receives nothing. You would need to carefully craft what amount of shares are initially minted to avoid significant rounding in requestWithdraw().

so withdrawRatio is not "strictly less than 1e18". So votiumWithdrawAmount = trackedvStrategyBalance and safEthWithdrawAmount = safEthBalance and the attacker can withdraw everything.

[c4-judge]

0xleastwood changed the severity to 2 (Med Risk)

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[romeroadrian]

@0xleastwood, I'm note sure where we misunderstand each other, but let me give a few example scenarios enabled by this issue:

1. The first user naively deposits with a _minout == 0 (because why bother when the (sans inflation) price() is known to start at 1e18?). The full normal inflation attack is possible and the attacker steals his entire deposit.
2. The first user wants to deposit 1 ETH and sets _minout to 95% of the price() (how else would he determine _minout?). The attacker can skim off the 5%.
3. The attacker inflates the price so that the rounding errors are the equivalent of $100. The first depositors experience little problem with this because they deposit large amounts in multiples of $100, so their rounding losses are only a few percent and don't revert. The attacker has perhaps already made a small profit now. However, now the protocol is established and user experience for smaller fish will be unbearable having to deposit in such large multiples or suffering rounding losses up to $100. The protocol loses reputation and may not be able to regain it after redeploying a new uninflated one.

Do you agree that any of these impacts warrants a HM grade? Do you agree that any of these scenarios are possible?

Honestly, this doesn't make sense at all. Why would the first user naively set 0 as minout? Why would they set a 5% (FIVE PERCENT!!) slippage on the initial deposit knowing the conditions? Seems like user mistake then, which should be QA.

[0xleastwood]

Honestly, this doesn't make sense at all. Why would the first user naively set 0 as minout? Why would they set a 5% (FIVE PERCENT!!) slippage on the initial deposit knowing the conditions? Seems like user mistake then, which should be QA.

The example scenarios are not valid imo, but the rounding precision loss indeed is. Basically, the first depositor can arbitrarily set an unfair exchange rate for votium tokens. Because of this, we can get significant rounding loss that is up to the exchange rate the attacker set.

Ultimately, smaller depositors could be severely limited in participating, but I would not expect users to incorrectly set _minout and lose out this way. It would be from the perspective that this user is unable to participate without accepting some rounding loss which means they would set _minout to a value lower than what should be expected.