Inflation attack in VotiumStrategy

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39

Vulnerability details

Summary

The VotiumStrategy contract is susceptible to the Inflation Attack, in which the first depositor can be front-runned by an attacker to steal their deposit.

Impact

Both AfEth and VotiumStrategy acts as vaults: accounts deposit some tokens and get back another token (share) that represents their participation in the vault.

These types of contracts are potentially vulnerable to the inflation attack: an attacker can front-run the initial deposit to the vault to inflate the value of a share and render the front-runned deposit worthless.

In AfEth, this is successfully mitigated by the slippage control. Any attack that inflates the value of a share to decrease the number of minted shares is rejected due to the validation of minimum output:

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L166-L167

166:         uint256 amountToMint = totalValue / priceBeforeDeposit;
167:         if (amountToMint < _minout) revert BelowMinOut();

However, this is not the case of VotiumStrategy. In this contract, no validation is done in the number of minted tokens. This means that an attacker can execute the attack by front-running the initial deposit, which may be from AfEth or from any other account that interacts with the contract. See Proof of Concept for a detailed walkthrough of the issue.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39-L46

39:     function deposit() public payable override returns (uint256 mintAmount) {
40:         uint256 priceBefore = cvxPerVotium();
41:         uint256 cvxAmount = buyCvx(msg.value);
42:         IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
43:         ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
44:         mintAmount = ((cvxAmount * 1e18) / priceBefore);
45:         _mint(msg.sender, mintAmount);
46:     }

Proof of Concept

Let's say a user wants to deposit in VotiumStrategy and calls deposit() sending an ETH amount such as it is expected to buy X tokens of CVX. Attacker will front-run the transaction and execute the following:

1. Initial state is empty contract, assets = 0 and supply = 0.
2. Attacker calls deposit with an amount of ETH such as to buy 1e18 CVX tokens, this makes assets = 1e18 and supply = 1e18.
3. Attacker calls requestWithdraw(1e18 - 1) so that supply = 1, assume also 1e18 - 1 CVX tokens are withdrawn so that cvxUnlockObligations = 1e18 - 1.
4. Attacker transfers (donates) X amount of CVX to VotiumStrategy contract.
5. At this point, priceBefore = cvxPerVotium() = (totalCvx - cvxUnlockObligations) * 1e18 / supply = (X + 1e18 - (1e18 - 1)) * 1e18 / 1 = (X + 1) * 1e18
6. User transaction gets through and deposit() buys X amount of CVX. Minted tokens will be mintAmount = X * 1e18 / priceBefore = X * 1e18 / (X + 1) * 1e18 = X / (X + 1) = 0.
7. User is then minted zero VotiumStrategy tokens.
8. Attacker calls requestWithdraw() again to queue withdrawal to remove all CVX balance from the contract, including the tokens deposited by the user.

Recommendation

There are multiple ways of solving the issue:

1. Similar to AfEth, add a minimum output check to ensure the amount of minted shares.
2. Track asset balances internally so an attacker cannot donate assets to inflate shares.
3. Mint an initial number of "dead shares", similar to how UniswapV2 does.

A very good discussion of these can be found here.

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as primary issue

[0xleastwood]

Downgrading this to medium severity because the _minOut parameter should actually prevent this attack as long as it's non-zero, but I agree this is of concern if users do not set this parameter. This is a stated assumption.

[c4-judge]

0xleastwood changed the severity to 2 (Med Risk)

[0xleastwood]

Oops, my bad, I mis-interpreted the first part of the issue.

[c4-judge]

0xleastwood changed the severity to 3 (High Risk)

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[0xleastwood]

Upon further investigation, AfEth.deposit() is not vulnerable to the deposit front-running. This is only an issue if we are interacting with the votium strategy contract directly which is atypical behaviour. However, funds are still at risk even with these stated assumptions so I believe medium severity to be more correct.

[c4-judge]

0xleastwood changed the severity to 2 (Med Risk)

[MiloTruck]

@0xleastwood apologies for commenting after post-judging QA, but isn't the inflation attack still a problem even if users only interact with the AfEth contract?

AfEth.deposit() calls VotiumStrategy's deposit() function, so if a user calls AfEth.deposit() after VotiumStrategy's state has been manipulated, vMinted will be 0, causing him to lose the portion of his ETH that was deposited into VotiumStrategy.

Unless I'm missing something here...

[0xleastwood]

@0xleastwood apologies for commenting after post-judging QA, but isn't the inflation attack still a problem even if users only interact with the AfEth contract?

AfEth.deposit() calls VotiumStrategy's deposit() function, so if a user calls AfEth.deposit() after VotiumStrategy's state has been manipulated, vMinted will be 0, causing him to lose the portion of his ETH that was deposited into VotiumStrategy.

Unless I'm missing something here...

Ultimately, I do believe _minOut to be sufficient in detecting such an attack.

[0xleastwood]

Actually, I don't even think this attack is honestly feasible, for the same reasons outlined in #58.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[0xleastwood]

Actually, I'd like to retract this, this is clearly possible when following the steps outlined in the POC.

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood marked the issue as selected for report

[elmutt]

we think this will solve it:

asymmetryfinance/afeth#179