Summary
The UpliftOnlyExample contract collects fees from swaps and stores them in the contract, but lacks any mechanism to withdraw these fees, resulting in permanent lockup of collected fees.
Vulnerability Details
https://github.com/Cyfrin/2024-12-quantamm/blob/a775db4273eb36e7b4536c5b60207c9f17541b92/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol#L342-L346
In the onAfterSwap function the fee is collected using _vault.sendTo to address(this) (UpliftOnlyExample contract). But there is no function to withdraw the collected fee and there is no mechanism to distribute the fee to the owner or admin which causes the fee to be permanently locked in the contract.
function onAfterSwap(AfterSwapParams calldata params) public override onlyVault returns (bool success, uint256 hookAdjustedAmountCalculatedRaw) {
if (hookFee > 0) {
_vault.sendTo(feeToken, address(this), ownerFee);
emit SwapHookFeeCharged(address(this), feeToken, ownerFee);
}
}
When swap occurs, fee is deducted and sent to the contract and then fee is accumulated in the contract balance. But there is no withdrawFees function or similar that causes the owner and admin can not access the fee and the fee is locked forever in the contract.
POC
Add this to UpliftExample.t.sol and run it forge test --match-test testFeeCollectionVulnerability -vvvv.
function testFeeCollectionVulnerability() public {
vm.prank(owner);
upliftOnlyRouter.setHookSwapFeePercentage(1e16);
uint256 initialRouterUsdcBalance = usdc.balanceOf(address(upliftOnlyRouter));
vm.startPrank(bob);
router.swapSingleTokenExactIn(
address(pool),
dai,
usdc,
1e21,
0,
type(uint256).max,
false,
""
);
vm.stopPrank();
uint256 expectedFee = 1e19;
assertEq(
usdc.balanceOf(address(upliftOnlyRouter)) - initialRouterUsdcBalance,
expectedFee,
"Fee not collected correctly"
);
vm.startPrank(owner);
(bool success,) = address(upliftOnlyRouter).call(
abi.encodeWithSignature("withdrawFees(address,uint256)", usdc, expectedFee)
);
assertFalse(success, "Should not be able to withdraw fees");
vm.stopPrank();
assertEq(
usdc.balanceOf(address(upliftOnlyRouter)),
expectedFee,
"Fees should remain in router"
);
vm.startPrank(alice);
(success,) = address(upliftOnlyRouter).call(
abi.encodeWithSignature("withdrawFees(address,uint256)", usdc, expectedFee)
);
assertFalse(success, "Non-owner should not be able to withdraw fees");
vm.stopPrank();
assertEq(
usdc.balanceOf(address(upliftOnlyRouter)),
expectedFee,
"Fees should still be stuck in router"
);
address quantAMMAdmin = updateWeightRunner.getQuantAMMAdmin();
assertEq(
usdc.balanceOf(quantAMMAdmin),
0,
"Protocol should not receive fees without withdrawal mechanism"
);
}
Trace:
├─ [582] USDC::balanceOf(upliftOnlyRouter: [0xF2E246BB76DF876Cef8b38ae84130F4F55De395b]) [staticcall]
│ └─ ← 10000000000000000000 [1e19]
Shows that a fee of 1% (10e18) of the swap was successfully collected on the upliftOnlyRouter contract.
├─ [210] upliftOnlyRouter::withdrawFees(USDC: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 10000000000000000000 [1e19])
│ └─ ← EvmError: Revert
Withdrawal attempts by the owner failed with a revert, proving there is no withdrawal mechanism.
├─ [210] upliftOnlyRouter::withdrawFees(USDC: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 10000000000000000000 [1e19])
│ └─ ← EvmError: Revert
Withdrawal attempts by non-owners also failed.
├─ [582] USDC::balanceOf(upliftOnlyRouter: [0xF2E246BB76DF876Cef8b38ae84130F4F55De395b]) [staticcall]
│ └─ ← 10000000000000000000 [1e19]
After the withdrawal attempt, the fee balance remains intact in the contract.
├─ [2582] USDC::balanceOf(vaultAdmin: [0x212224D2F2d262cd093eE13240ca4873fcCBbA3C]) [staticcall]
│ └─ ← 0
Shows that the protocol admin balance remains 0, no fee distribution.
This shows that fees are collected but cannot be accessed because there is no distribution mechanism to the protocol admin and no withdrawal function is implemented which causes fees to continue to accumulate and be locked forever in the contract.
Impact
Fee is permanently locked in the contract.
Tools Used
Recommendations
Implement Fee Withdrawal.
contract UpliftOnlyExample {
mapping(IERC20 => uint256) public collectedFees;
function withdrawFees(IERC20[] calldata tokens) external onlyOwner {
for (uint256 i = 0; i < tokens.length; i++) {
uint256 amount = collectedFees[tokens[i]];
if (amount > 0) {
collectedFees[tokens[i]] = 0;
uint256 protocolShare = amount / 2;
uint256 adminShare = amount - protocolShare;
tokens[i].safeTransfer(treasury, protocolShare);
tokens[i].safeTransfer(
updateWeightRunner.getQuantAMMAdmin(),
adminShare
);
emit FeeWithdrawn(
tokens[i],
treasury,
protocolShare,
adminShare
);
}
}
}
function onAfterSwap() {
if (ownerFee > 0) {
collectedFees[feeToken] += ownerFee;
_vault.sendTo(feeToken, address(this), ownerFee);
}
}
}
