Submission Details
Severity:
Lack of access control mechanism in `UpliftOnlyExample::onAfterRemoveLiquidity()` function.
Summary
Due to the lack of onlyVault modifier in UpliftOnlyExample::onAfterRemoveLiquidity() function, an attacker can call the function which is meant to be called by the vault only and grief the depositor/liquidity providers.
Vulnerability Details
The function UpliftOnlyExample::onAfterRemoveLiquidity() is a hook to be executed after removing the liquidity. The modifier onlySelfRouter is meaningless here as the caller can provide the address(this) and pass the restriction so it means the function is exposed to be called by anyone.
The attacker can call this function and grief the liquidity providers by removing liquidity providers' deposits and burning their LPNFT NFT token which was minted when the deposit was made. Another impact of this issue is that the innocent user will be forced to pay the minWithdraw fees.
function onAfterRemoveLiquidity(
address router,
address pool,
RemoveLiquidityKind,
uint256 bptAmountIn,
uint256[] memory,
uint256[] memory amountsOutRaw,
uint256[] memory,
bytes memory userData
) public override onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
address userAddress = address(bytes20(userData));
AfterRemoveLiquidityData memory localData = AfterRemoveLiquidityData({
pool: pool,
bptAmountIn: bptAmountIn,
amountsOutRaw: amountsOutRaw,
minAmountsOut: new uint256[](amountsOutRaw.length),
accruedFees: new uint256[](amountsOutRaw.length),
accruedQuantAMMFees: new uint256[](amountsOutRaw.length),
currentFee: minWithdrawalFeeBps,
feeAmount: 0,
prices: IUpdateWeightRunner(_updateWeightRunner).getData(pool),
lpTokenDepositValueNow: 0,
lpTokenDepositValueChange: 0,
lpTokenDepositValue: 0,
tokens: new IERC20[](amountsOutRaw.length),
feeDataArrayLength: 0,
amountLeft: 0,
feePercentage: 0,
adminFeePercent: 0
});
// We only allow removeLiquidity via the Router/Hook itself so that fee is applied correctly.
hookAdjustedAmountsOutRaw = amountsOutRaw;
//this rounding faxvours the LP
localData.lpTokenDepositValueNow = getPoolLPTokenValue(localData.prices, pool, MULDIRECTION.MULDOWN);
FeeData[] storage feeDataArray = poolsFeeData[pool][userAddress];
localData.feeDataArrayLength = feeDataArray.length;
localData.amountLeft = bptAmountIn;
for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {
localData.lpTokenDepositValue = feeDataArray[i].lpTokenDepositValue;
localData.lpTokenDepositValueChange =
(int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue)) /
int256(localData.lpTokenDepositValue);
uint256 feePerLP;
// if the pool has increased in value since the deposit, the fee is calculated based on the deposit value
if (localData.lpTokenDepositValueChange > 0) {
feePerLP =
(uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
10000;
}
// if the pool has decreased in value since the deposit, the fee is calculated based on the base value - see wp
else {
//in most cases this should be a normal swap fee amount.
//there always myst be at least the swap fee amount to avoid deposit/withdraw attack surgace.
feePerLP = (uint256(minWithdrawalFeeBps) * 1e18) / 10000;
}
// if the deposit is less than the amount left to burn, burn the whole deposit and move on to the next
if (feeDataArray[i].amount <= localData.amountLeft) {
uint256 depositAmount = feeDataArray[i].amount;
localData.feeAmount += (depositAmount * feePerLP);
localData.amountLeft -= feeDataArray[i].amount;
@> lpNFT.burn(feeDataArray[i].tokenID);
@> delete feeDataArray[i];
feeDataArray.pop();
if (localData.amountLeft == 0) {
break;
}
} else {
feeDataArray[i].amount -= localData.amountLeft;
localData.feeAmount += (feePerLP * localData.amountLeft);
break;
}
}
localData.feePercentage = (localData.feeAmount) / bptAmountIn;
hookAdjustedAmountsOutRaw = localData.amountsOutRaw;
localData.tokens = _vault.getPoolTokens(localData.pool);
localData.adminFeePercent = IUpdateWeightRunner(_updateWeightRunner).getQuantAMMUpliftFeeTake();
// Charge fees proportional to the `amountOut` of each token.
for (uint256 i = 0; i < localData.amountsOutRaw.length; i++) {
uint256 exitFee = localData.amountsOutRaw[i].mulDown(localData.feePercentage);
if (localData.adminFeePercent > 0) {
localData.accruedQuantAMMFees[i] = exitFee.mulDown(localData.adminFeePercent);
}
localData.accruedFees[i] = exitFee - localData.accruedQuantAMMFees[i];
hookAdjustedAmountsOutRaw[i] -= exitFee;
// Fees don't need to be transferred to the hook, because donation will redeposit them in the Vault.
// In effect, we will transfer a reduced amount of tokensOut to the caller, and leave the remainder
// in the pool balance.
}
if (localData.adminFeePercent > 0) {
_vault.addLiquidity(
AddLiquidityParams({
pool: localData.pool,
to: IUpdateWeightRunner(_updateWeightRunner).getQuantAMMAdmin(),
maxAmountsIn: localData.accruedQuantAMMFees,
minBptAmountOut: localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18,
kind: AddLiquidityKind.PROPORTIONAL,
userData: bytes("")
})
);
emit ExitFeeCharged(
userAddress,
localData.pool,
IERC20(localData.pool),
localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18
);
}
if (localData.adminFeePercent != 1e18) {
// Donates accrued fees back to LPs.
_vault.addLiquidity(
AddLiquidityParams({
pool: localData.pool,
to: msg.sender, // It would mint BPTs to router, but it's a donation so no BPT is minted
maxAmountsIn: localData.accruedFees, // Donate all accrued fees back to the pool (i.e. to the LPs)
minBptAmountOut: 0, // Donation does not return BPTs, any number above 0 will revert
kind: AddLiquidityKind.DONATION,
userData: bytes("") // User data is not used by donation, so we can set it to an empty string
})
);
}
return (true, hookAdjustedAmountsOutRaw);
}
Impact
Attacker can keep dossing depositors by calling the function.
The liquidity providers NFT gets burnt.
The LP will be forced to pay fees.
Tools Used
Manual Review
Recommendations
Add the modifier onlyVault() in the UpliftOnlyExample::onAfterRemoveLiquidity() function as recommended in the IHooks.sol
Updates
Lead Judging Commences
n0kto 10 months ago
Submission Judgement Published Assigned finding tags:
finding_onAfterRemoveLiquidity_no_access_control_wipe_all_data
Likelihood: High, anyone, anytime. Impact: High, Loss of funds
Prize pool breakdown
Total prize
49,600 OP
nSLOC
3,00017
OP / LOC
44,640
4,960
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.