Open c4-submissions opened 1 year ago
0xleastwood marked the issue as primary issue
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.
0xleastwood changed the severity to 2 (Med Risk)
Oops, my bad, I mis-interpreted the first part of the issue.
0xleastwood changed the severity to 3 (High Risk)
0xleastwood marked the issue as selected for report
elmutt (sponsor) confirmed
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.
0xleastwood changed the severity to 2 (Med Risk)
@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 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'sdeposit()
function, so if a user callsAfEth.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.
Actually, I don't even think this attack is honestly feasible, for the same reasons outlined in #58.
0xleastwood marked the issue as not selected for report
0xleastwood changed the severity to QA (Quality Assurance)
This previously downgraded issue has been upgraded by 0xleastwood
0xleastwood changed the severity to QA (Quality Assurance)
Actually, I'd like to retract this, this is clearly possible when following the steps outlined in the POC.
This previously downgraded issue has been upgraded by 0xleastwood
0xleastwood marked the issue as selected for report
we think this will solve it:
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
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
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 buyX
tokens of CVX. Attacker will front-run the transaction and execute the following:assets = 0
andsupply = 0
.1e18
CVX tokens, this makesassets = 1e18
andsupply = 1e18
.requestWithdraw(1e18 - 1)
so thatsupply = 1
, assume also1e18 - 1
CVX tokens are withdrawn so thatcvxUnlockObligations = 1e18 - 1
.priceBefore = cvxPerVotium() = (totalCvx - cvxUnlockObligations) * 1e18 / supply = (X + 1e18 - (1e18 - 1)) * 1e18 / 1 = (X + 1) * 1e18
deposit()
buys X amount of CVX. Minted tokens will bemintAmount = X * 1e18 / priceBefore = X * 1e18 / (X + 1) * 1e18 = X / (X + 1) = 0
.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:
A very good discussion of these can be found here.
Assessed type
Other