First Flight #12: Kitty Connect
First Flight #12: Kitty Connect
Beginner FriendlyFoundryNFTGameFi
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Inconsistent use of array length leads to impossible bridge or loss of NFT

n0kto

Description

bridgeNftToAnotherChain use userTokenIds.length - 1 to know the last owned ID of a user but it pops the array just before reusing this line.
If the user only have one NFT, it will underflow the condition below. Otherwise, if the sender have more than one NFT, and the bridged NFT is the second to last of the array, it will erase the last item without replacing the second-to-last.

function bridgeNftToAnotherChain(
...
) external {
...
uint256[] memory userTokenIds = s_ownerToCatsTokenId[msg.sender];
uint256 lastItem = userTokenIds[userTokenIds.length - 1];
​
@> s_ownerToCatsTokenId[msg.sender].pop();
​
@> if (idx < (userTokenIds.length - 1)) {
s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
}
...
}

Risk

Likelyhood: High

  • Occurs during every bridge if the Cat owner has only 1 NFT or bridges their second-to-last NFT.

Impact: High

  • Underflow and revert: No bridge possible

  • Removal of the wrong NFT from the array

Proof of Concept

  • Mint only one NFT to a Cat owner.

  • Attempt to bridge this NFT.

Recommended Mitigation

Use pop after replacing lastItem.

function bridgeNftToAnotherChain(
...
) external {
...
uint256[] memory userTokenIds = s_ownerToCatsTokenId[msg.sender];
uint256 lastItem = userTokenIds[userTokenIds.length - 1];
​
- s_ownerToCatsTokenId[msg.sender].pop();
​
if (idx < (userTokenIds.length - 1)) {
s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
}
+ s_ownerToCatsTokenId[msg.sender].pop();
...
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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