QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: low
Invalid

The delete keyword only resets array elements but does not remove them. Excessive use of the delete keyword results in unnecessary gas wastage.

theirrationalone

Summary

In the UpliftOnlyExample contract, the FeeData struct's state variable, poolsFeeData, is an array maintained as a mapping keyed by the pool address and owner address. This array is used to track various operations, including adding, updating, and removing liquidity, as well as minting and burning LPNFT.

In the onAfterRemoveLiquidity and afterUpdate functions, LPNFT is removed and transferred, respectively. However, during these operations, the FeeData is updated incorrectly.

What could be the root cause of this issue?

Vulnerability Details

See the code snippets below...

  1. UpliftOnlyExample::onAfterRemoveLiquidity:

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) {
.
.
...
​
for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {
.
.
...
// 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);
@> // @info: only resets array element
@> delete feeDataArray[i];
feeDataArray.pop();
​
if (localData.amountLeft == 0) {
break;
}
} else {
feeDataArray[i].amount -= localData.amountLeft;
localData.feeAmount += (feePerLP * localData.amountLeft);
break;
}
}
​
.
.
...
​
return (true, hookAdjustedAmountsOutRaw);
}

  1. UpliftOnlyExample::afterUpdate:

function afterUpdate(address _from, address _to, uint256 _tokenID) public {
.
.
...
​
if (tokenIdIndexFound) {
if (_to != address(0)) {
// Update the deposit value to the current value of the pool in base currency (e.g. USD) and the block index to the current block number
//vault.transferLPTokens(_from, _to, feeDataArray[i].amount);
feeDataArray[tokenIdIndex].lpTokenDepositValue = lpTokenDepositValueNow;
feeDataArray[tokenIdIndex].blockTimestampDeposit = uint32(block.number);
feeDataArray[tokenIdIndex].upliftFeeBps = upliftFeeBps;
​
//actual transfer not a afterTokenTransfer caused by a burn
poolsFeeData[poolAddress][_to].push(feeDataArray[tokenIdIndex]);
​
if (tokenIdIndex != feeDataArrayLength - 1) {
//Reordering the entire array could be expensive but it is the only way to keep true FILO
for (uint256 i = tokenIdIndex + 1; i < feeDataArrayLength; i++) {
delete feeDataArray[i - 1];
feeDataArray[i - 1] = feeDataArray[i];
}
}
​
@> // @q: why to reset first and delete the last element instead of just popping the last element off?
// ans: we can just remove(pop) the last element from the array
// hence we don't need reordering the array anymore
@> delete feeDataArray[feeDataArrayLength - 1];
feeDataArray.pop();
}
}
}

The delete keyword merely resets an array element. However, after resetting an element using the delete keyword, the element is immediately removed using the pop() array utility function. This raises the question: why reset the element first and then pop it? Resetting the element unnecessarily increases gas consumption, leading to inefficiency.

Impact

  1. Increased Gas Consumption:
    The delete keyword resets an array element to its default value (e.g., 0 for integers or false for booleans), but this operation still requires gas to execute. Immediately following that, calling pop() to remove the element from the array incurs additional gas costs. This double operation — resetting and then popping — leads to unnecessary gas wastage.
    In Ethereum, gas is a limited resource, and inefficient use can lead to higher transaction costs. Resetting the element first adds an extra, redundant gas cost that could be avoided.

  2. Increased Network Load:
    Ethereum transactions consume network resources. The more unnecessary actions (such as delete and pop() together) are taken in a contract, the more load is placed on the network, leading to slower transactions and higher costs for users.

Tools Used

Manual Review

Recommendations

Instead of using delete followed by pop(), it is better to just use pop() if the goal is to remove the element from the array. This would both reset the element and remove it from the array in a single, more efficient operation, saving on gas and improving contract performance.

Remove the reset lines of Code as we did below...

  1. UpliftOnlyExample::onAfterRemoveLiquidity:

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) {
.
.
...
​
for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {
.
.
...
// 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;
}
}
​
.
.
...
​
return (true, hookAdjustedAmountsOutRaw);
}

  1. UpliftOnlyExample::afterUpdate:

function afterUpdate(address _from, address _to, uint256 _tokenID) public {
.
.
...
​
if (tokenIdIndexFound) {
if (_to != address(0)) {
// Update the deposit value to the current value of the pool in base currency (e.g. USD) and the block index to the current block number
//vault.transferLPTokens(_from, _to, feeDataArray[i].amount);
feeDataArray[tokenIdIndex].lpTokenDepositValue = lpTokenDepositValueNow;
feeDataArray[tokenIdIndex].blockTimestampDeposit = uint32(block.number);
feeDataArray[tokenIdIndex].upliftFeeBps = upliftFeeBps;
​
//actual transfer not a afterTokenTransfer caused by a burn
poolsFeeData[poolAddress][_to].push(feeDataArray[tokenIdIndex]);
​
if (tokenIdIndex != feeDataArrayLength - 1) {
//Reordering the entire array could be expensive but it is the only way to keep true FILO
for (uint256 i = tokenIdIndex + 1; i < feeDataArrayLength; i++) {
delete feeDataArray[i - 1];
feeDataArray[i - 1] = feeDataArray[i];
}
}
​
- delete feeDataArray[feeDataArrayLength - 1];
feeDataArray.pop();
}
}
}
Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
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.

Support

FAQs

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

Give us feedback!