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

_nextTokenId should be initialised with 1 initially and it's not used in the contract. Oversight could make difficulties to track the NFT tokens and token supply verification.

theirrationalone

Summary

The _nextTokenId variable in UpliftOnlyExample.sol is declared but not used in the contract. According to the Natpec _nextTokenId should be initialised with 1 initially. The oversight could make difficulties to track the NFT tokens and token supply verification.

see the code snippet below:

UpliftOnlyExample.sol:

contract UpliftOnlyExample is MinimalRouter, BaseHooks, Ownable {
.
.
...
// NFT unique identifier.
// @info: forgot to use the _nextTokenId variable
// @info: the default value of _nextTokenId should be 1 not 0
uint256 private _nextTokenId;
.
.
...

Vulnerability Details

The _nextTokenId variable can be used in many places in the contract to track the NFT tokens and token supply verification.

see the code snippet below:

UpliftOnlyExample.sol::addLiquidityProportional:

function addLiquidityProportional(
address pool,
uint256[] memory maxAmountsIn,
uint256 exactBptAmountOut,
bool wethIsEth,
bytes memory userData
) external payable saveSender(msg.sender) returns (uint256[] memory amountsIn) {
.
.
...
nftPool[tokenID] = pool;
@> // @info: forgot to assign the tokenID to the _nextTokenId
// @info: it should be incremented by 1 and should be checked
// if the new tokenID is one less than the current _nextTokenId
}

UpliftOnlyExample.sol:::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 (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: not updating _nextTokenId here, it should be updated here
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);
}

Impact

  1. The default value of _nextTokenId is 0 which is wrong according to the Natpec. It should be initialised with 1 initially.

  2. The _nextTokenId variable is not used in the contract. The oversight could make difficulties to track the NFT tokens and token supply verification.

Tools Used

  1. Manual Review

Recommendations

Please initialise the _nextTokenId variable with 1 initially and use it in the contract to track the NFT tokens and token supply verification.

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!