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

`TokenDivider::divideNft` does not follow CEI pattern, leading to potential reentrancy attacks

marcoroccon

Description:
Every function in the smart contract should follow the CEI (Checks-Effects-Interactions) pattern, in order to avoid potential reentrancy attack from malicious actors. The CEI pattern is one of the best practices to follow when writing a smart contract, which provides that state variables should be updated before any interactions with external contracts.

Impact:
Reentrancy attacks are one of the most common attack vectors in smart contracts. An attacker could create a malicious smart contract which has some custom logic in the receive or fallback function, which are triggered when receiving native tokens. It is crucial to avoid smart contracts to reenter functions before their completion.

Tools Used:
Slither, manual review

Recommended Mitigation:
It is recommended to rephrase the code of TokenDivider::divideNft function, in order to relfect the above mentioned CEI pattern.

function divideNft(address nftAddress, uint256 tokenId, uint256 amount) onlyNftOwner(nftAddress, tokenId) onlyNftOwner(nftAddress ,tokenId) external {
​
if(nftAddress == address(0)) { revert TokenDivider__NftAddressIsZero(); }
if(amount == 0) { revert TokenDivider__AmountCantBeZero(); }
​
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);
​
- IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
​
- if(IERC721(nftAddress).ownerOf(tokenId) == msg.sender) { revert TokenDivider__NftTransferFailed(); }
​
balances[msg.sender][erc20] = amount;
nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
erc20ToMintedAmount[erc20] = amount;
erc20ToNft[erc20] = nftAddress;
​
+ erc20Contract.mint(address(this), amount);
+ IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
​
- emit NftDivided(nftAddress, amount, erc20);
bool transferSuccess = IERC20(erc20).transfer(msg.sender, amount);
if(!transferSuccess) {
revert TokenDivider__TransferFailed();
}
​
+ emit NftDivided(nftAddress, amount, erc20);
}

Additionally, to increase the security level of the contract, TokenDivider could inherit from ReentrancyGuard contract from OpenZeppelin, which already has a battle-tested nonReentrant modifier approved by the community to be added in TokenDivider::divideNft.

For reference, see ReentrancyGuard.sol

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.5/contracts/security/ReentrancyGuard.sol

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.