QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: high
Valid

`onAfterRemoveLiquidity` in the UpliftOnlyExample lacks proper access control and can be used to reduce all liquidity providers balance to zero

0xacnologiac

Summary

The onAfterRemoveLiquidity lacks the required access control, and can be used by any malicious attacker to reduce all liquidity providers balance to zero while stealing part of the fees as profit

Vulnerability Details

The UpliftOnlyExample is both the hook and router contract used to manage funds, however there is a critical issue here which enables the theft of all liquidity added.
The function as shown below does not check to ensure the function call is coming from the Vault contract before modifying state, burning the user's nft, or deducting fees.

//@audit - this function is vulnerable, anybody can call it by passing in the router address
function onAfterRemoveLiquidity(
address router,
address pool,
RemoveLiquidityKind,
uint256 bptAmountIn,
uint256[] memory,
uint256[] memory amountsOutRaw,
uint256[] memory,
bytes memory userData
) public override onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
address userAddress = address(bytes20(userData));

The function can be used to reduce the user's balance which is stored in the poolsFeeData mapping, When the whole liquidity in 1 FeeData Struct is being removed, the NFT is burned from the liquidity Providers wallet.

// if the deposit is less than the amount left to burn, burn the whole deposit and move on to the next
if (feeDataArray[i].amount <= localData.amountLeft) {
uint256 depositAmount = feeDataArray[i].amount;
localData.feeAmount += (depositAmount * feePerLP);
​
localData.amountLeft -= feeDataArray[i].amount;
​
lpNFT.burn(feeDataArray[i].tokenID);
​
delete feeDataArray[i];
feeDataArray.pop();
​
if (localData.amountLeft == 0) {
break;
}
} else {
feeDataArray[i].amount -= localData.amountLeft;
localData.feeAmount += (feePerLP * localData.amountLeft);
break;
}

Conditions for these is if the quantAMMSwapFeeTake is ever zero, however the impact for this is too large even for an edge scenario

Impact

  • All users who added liquidity using this UpLift contract will loose all funds.

  • The bulk of the liquidity is permanently locked away since they now have no owner, and the vault will always call onAfterRemoveLiquidity() when withdrawing.

  • The part of the fees accrued that do not go to the QuantAMMAdmin are sent to the attacker instead of being deposited back to LPs.

Tools Used

Manual Review

Recommended Mitigation

Adding the onlyVault is necessary in this function. Add it as a modifier to the function.

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_onAfterRemoveLiquidity_no_access_control_wipe_all_data

Likelihood: High, anyone, anytime. Impact: High, Loss of funds

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!