Summary
The removeLiquidityProportional function in the UpliftOnlyExample contract contains a vulnerability that allows an attacker to bypass ownership checks by making a minimal deposit. This enable the attacker to remove liquidity they do not own.
Vulnerability Details
The removeLiquidityProportional function checks if the caller has any deposits in the specified pool by verifying the length of the poolsFeeData array for the caller. However, an attacker can bypass this check by making a minimal deposit, which allows them to pass the depositLength check and attempt to remove liquidity they do not own.
https://github.com/Cyfrin/2024-12-quantamm/blob/a775db4273eb36e7b4536c5b60207c9f17541b92/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol#L265-L286
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);
}
amountsOut = _removeLiquidityProportional(
pool,
address(this),
msg.sender,
bptAmountIn,
minAmountsOut,
wethIsEth,
abi.encodePacked(msg.sender)
);
}
An attacker can exploit this vulnerability by making a minimal deposit and then attempting to remove more liquidity than they own.
pragma solidity >=0.8.24;
import "forge-std/Test.sol";
import "../src/UpliftOnlyExample.sol";
contract UpliftOnlyExampleTest is Test {
UpliftOnlyExample upliftOnlyExample;
address attacker = address(0x123);
address victim = address(0x456);
address pool = address(0x789);
function setUp() public {
upliftOnlyExample = new UpliftOnlyExample(...);
vm.prank(victim);
upliftOnlyExample.addLiquidityProportional(pool, ..., 1000, false, "");
vm.prank(attacker);
upliftOnlyExample.addLiquidityProportional(pool, ..., 1, false, "");
}
function testUnauthorizedLiquidityRemoval() public {
vm.prank(attacker);
upliftOnlyExample.removeLiquidityProportional(1000, ..., false, pool);
uint256 attackerBalance = upliftOnlyExample.getUserPoolFeeData(pool, attacker).length;
assertEq(attackerBalance, 0, "Attacker should not be able to remove victim's liquidity");
}
}
Impact
The vulnerability allows an attacker to remove liquidity they do not own, potentially leading to unauthorized loss of funds for legitimate users. This can undermine the integrity of the pool and erode user trust.
Tools Used
Manual review.
Recommendations
To mitigate this issue, additional checks should be added to ensure that the amount of liquidity being removed corresponds to the caller's actual deposits. Here is the updated function with additional checks:
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);
}
uint256 totalUserLiquidity = 0;
for (uint256 i = 0; i < depositLength; i++) {
totalUserLiquidity += poolsFeeData[pool][msg.sender][i].amount;
}
if (bptAmountIn > totalUserLiquidity) {
revert WithdrawalByNonOwner(msg.sender, pool, bptAmountIn);
}
amountsOut = _removeLiquidityProportional(
pool,
address(this),
msg.sender,
bptAmountIn,
minAmountsOut,
wethIsEth,
abi.encodePacked(msg.sender)
);
uint256 remainingAmount = bptAmountIn;
for (uint256 i = 0; i < depositLength && remainingAmount > 0; i++) {
if (poolsFeeData[pool][msg.sender][i].amount <= remainingAmount) {
remainingAmount -= poolsFeeData[pool][msg.sender][i].amount;
delete poolsFeeData[pool][msg.sender][i];
} else {
poolsFeeData[pool][msg.sender][i].amount -= remainingAmount;
remainingAmount = 0;
}
}
for (uint256 i = 0; i < depositLength; i++) {
if (poolsFeeData[pool][msg.sender][i].amount == 0) {
for (uint256 j = i; j < depositLength - 1; j++) {
poolsFeeData[pool][msg.sender][j] = poolsFeeData[pool][msg.sender][j + 1];
}
poolsFeeData[pool][msg.sender].pop();
depositLength--;
i--;
}
}
}
