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

State Updates After External Contract Calls

swissahman

Summary

The claimNft function in the TokenDivider contract performs state updates after making external contract calls, violating the checks-effects-interactions pattern. This sequence creates a critical vulnerability where the contract's state could become inconsistent if external calls fail or are manipulated.

Vulnerability Detail

Current implementation shows state updates occurring after external contract interactions:

function claimNft(address nftAddress) external {
ERC20Info storage tokenInfo = nftToErc20Info[nftAddress];
// EXTERNAL CALL #1
ERC20ToGenerateNftFraccion(tokenInfo.erc20Address).burnFrom(
msg.sender,
erc20ToMintedAmount[tokenInfo.erc20Address]
);
​
// STATE UPDATES after external call (VULNERABLE)
balances[msg.sender][tokenInfo.erc20Address] = 0;
erc20ToMintedAmount[tokenInfo.erc20Address] = 0;
​
// EXTERNAL CALL #2
IERC721(nftAddress).safeTransferFrom(
address(this),
msg.sender,
tokenInfo.tokenId
);
}

Impact

This vulnerability could result in:

  1. Inconsistent contract state if external calls fail

  2. Loss of user tokens if state updates persist after failed transfers

  3. Potential double-spending vulnerabilities

  4. Permanent locking of NFTs in the contract

  5. Manipulation of contract state through malicious external contracts

Tool Used

Foundry

Proof of Concept

contract StateManipulationAttacker {
TokenDivider target;
bool shouldRevert;
constructor(address _target) {
target = TokenDivider(_target);
}
function attackClaim(address nftAddress) external {
shouldRevert = true;
target.claimNft(nftAddress);
}
function onERC721Received(
address,
address,
uint256,
bytes calldata
) external returns (bytes4) {
if(shouldRevert) {
// Revert after state changes but during NFT transfer
revert("Malicious revert");
}
return this.onERC721Received.selector;
}
}

Attack Scenario

  1. User calls claimNft with valid parameters

  2. ERC20 tokens are successfully burned

  3. Contract updates state variables

  4. During NFT transfer, the receiving contract reverts

  5. The function call fails, but state updates remain:

    • ERC20 tokens are burned

    • State indicates the claim is complete

    • NFT remains locked in the contract

    • User loses their tokens with no recourse

Recommended Mitigation Steps

  1. Implement Checks-Effects-Interactions Pattern

function claimNft(
address nftAddress
) external nonReentrant {
// 1. CHECKS
ERC20Info storage tokenInfo = _validateClaimRequest(nftAddress);
uint256 totalSupply = erc20ToMintedAmount[tokenInfo.erc20Address];
// 2. EFFECTS - Update state first
_updateStateForClaim(nftAddress, tokenInfo.erc20Address);
// 3. INTERACTIONS - External calls last
_executeClaimTransfers(tokenInfo, totalSupply);
}

  1. Add State Management Functions

function _validateClaimRequest(
address nftAddress
) private view returns (ERC20Info storage) {
ERC20Info storage tokenInfo = nftToErc20Info[nftAddress];
require(tokenInfo.erc20Address != address(0), "NFT not fractionalized");
uint256 totalSupply = erc20ToMintedAmount[tokenInfo.erc20Address];
require(
balances[msg.sender][tokenInfo.erc20Address] == totalSupply,
"Insufficient balance"
);
return tokenInfo;
}
​
function _updateStateForClaim(
address nftAddress,
address erc20Address
) private {
uint256 totalSupply = erc20ToMintedAmount[erc20Address];
// Clear all relevant state
delete balances[msg.sender][erc20Address];
delete erc20ToMintedAmount[erc20Address];
delete erc20ToNft[erc20Address];
delete nftToErc20Info[nftAddress];
emit StateUpdated(nftAddress, erc20Address, totalSupply);
}
​
function _executeClaimTransfers(
ERC20Info memory tokenInfo,
uint256 totalSupply
) private {
// Execute token burns and transfers
bool burnSuccess = ERC20ToGenerateNftFraccion(tokenInfo.erc20Address)
.burnFrom(msg.sender, totalSupply);
require(burnSuccess, "Burn failed");
IERC721(nftAddress).safeTransferFrom(
address(this),
msg.sender,
tokenInfo.tokenId
);
emit NftClaimed(nftAddress, msg.sender, tokenInfo.tokenId);
}

  1. Implement Transaction Guards

contract TokenDivider {
mapping(bytes32 => bool) private _pendingTx;
modifier guardedTransaction(bytes32 txId) {
require(!_pendingTx[txId], "Transaction in progress");
_pendingTx[txId] = true;
_;
delete _pendingTx[txId];
}
}

  1. Add Recovery Mechanism

function recoverFailedClaim(
address nftAddress,
address erc20Address
) external onlyOwner {
require(
IERC721(nftAddress).ownerOf(tokenInfo.tokenId) == address(this),
"NFT not in contract"
);
// Restore state if necessary
_restoreClaimState(nftAddress, erc20Address);
}

Additional Recommendations

  1. Implement comprehensive event logging

  2. Add emergency pause functionality

  3. Consider using a state machine pattern

  4. Implement claim timeouts

  5. Add circuit breaker pattern for repeated failures

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.