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

Improper Order of Operations in divideNft Function

teoslaf

Summary

The divideNft() function does not follow the Checks-Effects-Interactions (CEI) pattern correctly, leading to potential state inconsistencies and misleading event emissions.

Vulnerability Details

  1. External Calls Before State Updates

The function updates the state (e.g., token balances and mappings) before confirming that the final ERC20 token transfer to the caller succeeds. If the transfer fails, the transaction reverts, but the state update would have already occurred, causing inconsistencies.

  1. Event Emission Before Final Confirmation

The NftDivided event is emitted before the function ensures that all operations, including the ERC20 transfer, are successful. This premature emission can mislead off-chain services and users into believing the operation succeeded when it did not.

Impact

  • State Inconsistencies: Partial updates to state if the function reverts after state changes but before the token transfer succeeds.

  • Misleading Event Logs: Incorrect event emissions, causing confusion for users and off-chain systems reliant on event data.

Tools Used

Manual code review.

Recommendations

Reorder the function to follow the CEI pattern strictly:

  1. Perform all external interactions (e.g., NFT transfer and ERC20 token transfer) before any state updates.

  2. Update the contract’s state only after confirming the success of all external calls.

  3. Emit events last, only after the entire operation completes successfully.

function divideNft(address nftAddress, uint256 tokenId, uint256 amount) external {
// Checks
if (nftAddress == address(0)) {
revert TokenDivider__NftAddressIsZero();
}
if (amount == 0) {
revert TokenDivider__AmountCantBeZero();
}
​
// Interactions: Transfer NFT first
IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
if (IERC721(nftAddress).ownerOf(tokenId) != address(this)) {
revert TokenDivider__NftTransferFailed();
}
​
// Interactions: Create and mint tokens
ERC20ToGenerateNftFraccion erc20Contract = new ERC20ToGenerateNftFraccion(
string(abi.encodePacked(ERC721(nftAddress).name(), "Fraccion")),
string(abi.encodePacked("F", ERC721(nftAddress).symbol()))
);
erc20Contract.mint(address(this), amount);
address erc20 = address(erc20Contract);
​
// Interactions: Transfer tokens to the sender
bool transferSuccess = IERC20(erc20).transfer(msg.sender, amount);
if (!transferSuccess) {
revert TokenDivider__TransferFailed();
}
​
// Effects: Update state after successful transfers
balances[msg.sender][erc20] = amount;
nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
erc20ToMintedAmount[erc20] = amount;
erc20ToNft[erc20] = nftAddress;
​
// Emit event last
emit NftDivided(nftAddress, amount, erc20);
}
​
Updates

Lead Judging Commences

fishy Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Reentrancy

Appeal created

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

Support

FAQs

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