QuantAMM

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

Inconsistent State of Missing Fee Data After NFT Transfer in `UpliftOnlyExample`

Summary

The UpliftOnlyExample contract's afterUpdate function, which is triggered after an LPNFT transfer, assumes that a corresponding FeeData entry will always exist for the transferred NFT's tokenID within the recipient's feeDataArray for the relevant pool. However, this assumption can be violated if there's an error in the NFT transfer logic or if the system can reach an unexpected state where an NFT exists without associated fee data. If the FeeData entry is not found, the function does not revert, leading to an inconsistent state where a valid NFT holder might not have the necessary data for proper fee calculation upon withdrawal. This can result in incorrect fee calculations, potential denial of service, or the possibility of exploitation in conjunction with other vulnerabilities. The root cause is likely an error in the NFT transfer or FeeData management logic.

Vulnerability Details

The vulnerability arises in the afterUpdate function of the UpliftOnlyExample contract which is called after an LPNFT transfer is completed via the overridden _update function in the LPNFT contract LPNFT.sol#L49-L56. The core issue is an assumption that a corresponding FeeData entry will always exist for a given NFT's tokenID within the recipient's feeDataArray after a transfer.

function _update(address to, uint256 tokenId, address auth) internal override returns (address previousOwner) {
previousOwner = super._update(to, tokenId, auth);
//_update is called during mint, burn and transfer. This functionality is only for transfer
if (to != address(0) && previousOwner != address(0)) {
//if transfering the record in the vault needs to be changed to reflect the change in ownership
router.afterUpdate(previousOwner, to, tokenId);
}
}

The afterUpdate function iterates through the sender's feeDataArray (poolsFeeData[poolAddress][msg.sender]) to locate the FeeData struct associated with the transferred NFT's tokenID (UpliftOnlyExample.sol#L597-L603):

for (uint256 i; i < feeDataArrayLength; ++i) {
if (feeDataArray[i].tokenID == _tokenID) {
tokenIdIndex = i;
tokenIdIndexFound = true;
break;
}
}

It expects to find a match, updates the entry's fields, and then copies this entry to the recipient's feeDataArray (poolsFeeData[poolAddress][_to]). Finally, it removes the entry from the sender's array and reorders the array to maintain FILO order (UpliftOnlyExample.sol#L605-L627):

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 (uint i = tokenIdIndex + 1; i < feeDataArrayLength; i++) {
delete feeDataArray[i - 1];
feeDataArray[i - 1] = feeDataArray[i];
}
}
delete feeDataArray[feeDataArrayLength - 1];
feeDataArray.pop();
}
}

However, if the tokenID is not found in the sender's feeDataArray (i.e., tokenIdIndexFound remains false after the loop), the function does not revert. This implies that the system can enter a state where a user holds a valid NFT (transferred to them) but does not have the corresponding FeeData entry in their poolsFeeData mapping for the relevant pool.

This situation can occur if there is a bug in the NFT transfer logic that fails to correctly manage the FeeData entries, or if there is another way to manipulate the system state such that an NFT can exist without its associated FeeData being properly created or transferred.

The absence of a FeeData entry for a valid NFT holder can lead to incorrect fee calculations during withdrawals, potentially causing a denial of service if the withdrawal process relies on this data or a miscalculation of fees owed. The code in LPNFT._update relies on UpliftOnlyExample.afterUpdate for updating the fee data. Thus, any issues here directly affect the LPNFT contract's operation.

Impact

The absence of a corresponding FeeData entry for a valid NFT holder leads to the following impacts:

  1. Data Inconsistency: The primary impact is an inconsistent state within the UpliftOnlyExample contract's accounting. The poolsFeeData mapping is intended to accurately reflect the deposit history and fee-related information for each user and pool. When an NFT is transferred, the associated FeeData should also be transferred. If this fails, the mapping no longer accurately represents the on-chain reality.

  2. Incorrect Fee Calculation: The FeeData entry contains critical information for calculating withdrawal fees, including the initial deposit value, deposit timestamp, and applicable uplift fee basis points. If this data is missing, the onAfterRemoveLiquidity function in which relies on it for fee calculations, will not be able to calculate the correct fee.

  3. Potential for Exploitation: While a direct exploit might not be immediately obvious, an inconsistent state can create opportunities for malicious actors to gain an unfair advantage. For example, if an attacker can manipulate the system to transfer an NFT without transferring the FeeData, they might be able to avoid paying fees on withdrawal or manipulate the fee calculation in their favor. The exact exploit would depend on the specific way the missing FeeData is handled (or not handled) by the withdrawal logic.

  4. Reputational Damage: Incorrect fee calculations and failed withdrawals can damage the protocol's reputation and erode user trust.

Given these potential consequences, the severity of this vulnerability is best classified as Medium. It does not lead to an immediate and direct loss of funds in all cases, but it creates a serious data inconsistency and opens the door for potential issues and exploits.

Tools Used

Manual Review

Recommendations

The provided code revision, which adds a require statement to revert if tokenIdIndexFound is false in UpliftOnlyExample.afterUpdate, effectively addresses the immediate vulnerability. By ensuring that a FeeData entry must exist for the transferred NFT, the function prevents the system from entering an inconsistent state:

for (uint256 i; i < feeDataArrayLength; ++i) {
if (feeDataArray[i].tokenID == _tokenID) {
tokenIdIndex = i;
tokenIdIndexFound = true;
break;
}
}
+ require(tokenIdIndexFound == true, "UpliftOnlyExample: FeeData entry not found for transferred NFT");
- 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 (uint 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 4 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.