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

freeze of funds (fees) to `QuantAMMAdmin` in `onAfterRemoveLiquidity()` fees distribution

huntoor

Summary

in UpliftOnlyExample::onAfterRemoveLiquidity() the function sends the QuantAMMAdmin to the admin in the wrong way, making it non retrievable for the admin

Vulnerability Details

in UpliftOnlyExample::onAfterRemoveLiquidity(), the fees are sent the to QuantAMMAdmin as a BPT tokens here

File: UpliftOnlyExample.sol
431: function onAfterRemoveLiquidity(
​
536: if (localData.adminFeePercent > 0) {
537: _vault.addLiquidity(
538: AddLiquidityParams({
539: pool: localData.pool,
540: to: IUpdateWeightRunner(_updateWeightRunner).getQuantAMMAdmin(),
541: maxAmountsIn: localData.accruedQuantAMMFees,
542: minBptAmountOut: localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18,
543: kind: AddLiquidityKind.PROPORTIONAL,
544: userData: bytes("")
​

But the problem is that when the admin wants to exchange those BPT tokens, he can't do it through UpliftOnlyExample directly, since the admin doesn't have LPNFT token or a stored position in poolsFeeData failing his txn in onAfterRemoveLiquidity by this loop that will underflow if he doesn't have enough BPT token balance registered in the contract here

471: for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {

this will underflow cause his desired withdrawal balance haven't been substracted during the loop to break it here

504: if (localData.amountLeft == 0) {
505: break;
506: }
507: } else {
508: feeDataArray[i].amount -= localData.amountLeft;
509: localData.feeAmount += (feePerLP * localData.amountLeft);
510: break;
511: }

And if he tries to remove directly from BalancerV3 vault, he can't since balancer calls onAfterRemoveLiquidity with msg.sender as the router, which will fail due to onlySelfRouter modifier

File: Vault.sol
846: // Uses msg.sender as the Router (the contract that called the Vault).
847: if (poolData.poolConfigBits.shouldCallAfterRemoveLiquidity()) {
848: // `hooksContract` needed to fix stack too deep.
849: IHooks hooksContract = _hooksContracts[params.pool];
850:
851: amountsOut = poolData.poolConfigBits.callAfterRemoveLiquidityHook(
852: msg.sender,
853: amountsOutScaled18,
854: amountsOut,
855: bptAmountIn,
856: params,
857: poolData,
858: hooksContract
859: );
860: }
​
................
​
File: UpliftOnlyExample.sol
431: function onAfterRemoveLiquidity(
432: address router,
433: address pool,
434: RemoveLiquidityKind,
435: uint256 bptAmountIn,
436: uint256[] memory,
437: uint256[] memory amountsOutRaw,
438: uint256[] memory,
439: bytes memory userData
440: ) public override onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
​
.................
​
190: modifier onlySelfRouter(address router) {
191: _ensureSelfRouter(router);
192: _;
193: }
..................
​
683: function _ensureSelfRouter(address router) private view {
684: if (router != address(this)) {
685: revert CannotUseExternalRouter(router);
686: }
687: }
​

This way, the admin is holding useless BPT tokens that can't be exchanged to tokens

Impact

Loss of funds (fees) to QuantAMMAdmin

Tools Used

Manual Review

Recommendations

  • Implement position registering to admin and mint NFT to him (complex and gas costly)

  • Implement a logic to send Tokens to admin and not adding liquidity to him (easier and less gas costly)

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_Uplift_admin_cannot_withdraw_without_nft

Likelihood: High, won’t be able to withdraw. Impact: High, funds stuck.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!