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

improper access control in `onAfterRemoveLiquidity()` risks loss of funds by attacker

0xhuntoor

Summary

in UpliftOnlyExample::onAfterRemoveLiquidity() absent access control to only allow the vault calls opens up a path to cause loss of funds to LP providers by an attacker calling to with victim address to burn the victim NFTs

Vulnerability Details

onAfterRemoveLiquidity is not access controlled and can be called by any one as long as the passed router is the UpliftOnlyExample it self, which is false access control guard.

Also the function takes the user address that his NFT will be burnt as a passed variable.

What makes it worse is that LPNFT::Burn() doesn't have checks for tokens approvals or ownership, so UpliftOnlyExample can burn any user NFT

Here is what can happen:
1- Attacker calls Unlock() on balancerV3 vault to have a transient unlock of the vault for the txn
2- Attacker calls onAfterRemoveLiquidity after calling unlock passing the:

  • router as UpliftOnlyExample to pass the onlySelfRouter modifier

  • userData to be the victim address

  • bptAmountIn to be the balance registered in poolsFeeData for the victim

3- The NFTs of the victim will be burnt due to this block

File: UpliftOnlyExample.sol
493: if (feeDataArray[i].amount <= localData.amountLeft) {
494: uint256 depositAmount = feeDataArray[i].amount;
495: localData.feeAmount += (depositAmount * feePerLP);
496:
497: localData.amountLeft -= feeDataArray[i].amount;
498:
499: lpNFT.burn(feeDataArray[i].tokenID);
500:
501: delete feeDataArray[i];
502: feeDataArray.pop();
503:
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: }

4- Attacker will need to call settle() to settle debt taken from the UpliftOnlyExample call to addLiquidity() (the fee distribution Logic)

File: UpliftOnlyExample.sol
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("")
545: })
546: );
547: emit ExitFeeCharged(
548: userAddress,
549: localData.pool,
550: IERC20(localData.pool),
551: localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18
552: );
553: }
554:
555: if (localData.adminFeePercent != 1e18) {
556: // Donates accrued fees back to LPs.
557: _vault.addLiquidity(
558: AddLiquidityParams({
559: pool: localData.pool,
560: to: msg.sender, // It would mint BPTs to router, but it's a donation so no BPT is minted
561: maxAmountsIn: localData.accruedFees, // Donate all accrued fees back to the pool (i.e. to the LPs)
562: minBptAmountOut: 0, // Donation does not return BPTs, any number above 0 will revert
563: kind: AddLiquidityKind.DONATION,
564: userData: bytes("") // User data is not used by donation, so we can set it to an empty string
565: })
566: );
567: }
​
.....................
​
File: Vault.sol
507: function addLiquidity(
569: (amountsIn, amountsInScaled18, bptAmountOut, returnData) = _addLiquidity(
570: poolData,
571: params,
572: maxAmountsInScaled18
573: );
)
​
.....................
​
610: function _addLiquidity(
732: // 3) Deltas: Debit of token[i] for amountInRaw.
733: _takeDebt(token, amountInRaw);
734: )
​
​

The loss for attacker is very smaller (fees of minimumSwapFees or UpLiftFee) compared to the complete loss of funds for the victim, since users can only retrieve funds throughremoveLiquidityProportional by their NFT and the poolsFeeData

Impact

Complete loss of funds

Tools Used

Manual Review

Recommendations

Add OnlyVault modifier

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_onAfterRemoveLiquidity_no_access_control_wipe_all_data

Likelihood: High, anyone, anytime. Impact: High, Loss of funds

Support

FAQs

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