Submission Details
Severity:
UpliftOnlyExample.sol :: onAfterRemoveLiquidity() nftPool mapping is not cleared when the entire liquidity of an NFT is removed and the NFT is burned.
Summary
onAfterRemoveLiquidity() is used to remove liquidity. The issue arises when the entire liquidity associated with an NFT is removed, and the NFT is burned the nftPool mapping is not cleared.
Vulnerability Details
In addLiquidityProportional(), when an NFT is minted and linked to a pool, it is recorded in the nftPool mapping.
function addLiquidityProportional(
address pool,
uint256[] memory maxAmountsIn,
uint256 exactBptAmountOut,
bool wethIsEth,
bytes memory userData
) external payable saveSender(msg.sender) returns (uint256[] memory amountsIn) {
if (poolsFeeData[pool][msg.sender].length > 100) {
revert TooManyDeposits(pool, msg.sender);
}
// Do addLiquidity operation - BPT is minted to this contract.
amountsIn = _addLiquidityProportional(
pool,
msg.sender,
address(this),
maxAmountsIn,
exactBptAmountOut,
wethIsEth,
userData
);
uint256 tokenID = lpNFT.mint(msg.sender);
//this requires the pool to be registered with the QuantAMM update weight runner
//as well as approved with oracles that provide the prices
uint256 depositValue = getPoolLPTokenValue(
IUpdateWeightRunner(_updateWeightRunner).getData(pool),
pool,
MULDIRECTION.MULDOWN
);
poolsFeeData[pool][msg.sender].push(
FeeData({
tokenID: tokenID,
amount: exactBptAmountOut,
//this rounding favours the LP
lpTokenDepositValue: depositValue,
//known use of timestamp, caveats are known.
blockTimestampDeposit: uint40(block.timestamp),
upliftFeeBps: upliftFeeBps
})
);
@> nftPool[tokenID] = pool;
}
However, in onAfterRemoveLiquidity(), when the entire liquidity of an NFT is removed, the nftPool mapping is not cleared.
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];
@> //@audit-issue nftPool[tokenID] is not cleared
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
When the entire liquidity of an NFT is removed, the nftPool mapping is not cleared, causing it to remain associated with the pool despite no longer having any actual liquidity.
Tools Used
Manual review.
Recommendations
To resolve the issue, clear the nftPool mapping when the entire liquidity of an NFT is removed from a pool.
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 nftPool[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);
}
Updates
Lead Judging Commences
n0kto 10 months ago
Submission Judgement Published Reason: Non-acceptable severity
Assigned finding tags:
Informational or Gas / Admin is trusted / Pool creation is trusted / User mistake / Suppositions
Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelyhood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.
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.