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

Low-Severity Findings Report

0xsnoweth

Summary

This report compiles all low-severity findings identified during the audit of the TokenDivider and ERC20ToGenerateNftFraccion contracts. These findings include issues related to code redundancy, state variable ordering, reentrancy risks, function visibility, naming conventions, missing events, and magic numbers. While these issues do not pose immediate risks to funds or protocol functionality, they impact code quality, maintainability, and gas efficiency. Recommendations are provided to address each finding.

Vulnerability Details

[L-1] Redundant Use of onlyNftOwner Modifier in TokenDivider::divideNft

Description: The onlyNftOwner modifier is applied twice in the divideNft function, adding unnecessary overhead and reducing code readability.

Impact: Increases gas costs and reduces maintainability.

Proof of Concept:

function divideNft(address nftAddress, uint256 tokenId, uint256 amount)
onlyNftOwner(nftAddress, tokenId)
onlyNftOwner(nftAddress, tokenId)
external
{
// Function logic
}

Recommendation:
Remove the duplicate modifier:

function divideNft(address nftAddress, uint256 tokenId, uint256 amount)
onlyNftOwner(nftAddress, tokenId)
external
{
// Function logic
}

[L-2] State Variables Written After External Calls in TokenDivider::divideNft

Description: State variables are updated after an external call to safeTransferFrom, increasing the risk of incorrect state if the call fails or behaves maliciously.

Impact: Potential for incorrect state and reentrancy risks.

Proof of Concept:

IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
balances[msg.sender][erc20] = amount;
nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});

Recommendation: Update state variables before external calls and implement reentrancy guards.

[L-3] Potential Reentrancy Attack in TokenDivider::divideNft

Description: External calls to transfer are made before state updates, exposing the contract to reentrancy attacks.

Impact: Malicious actors could manipulate state variables or steal tokens.

Proof of Concept:

bool transferSuccess = IERC20(erc20).transfer(msg.sender, amount);
if (!transferSuccess) {
revert TokenDivider__TransferFailed();
}

Recommendation: Update state variables before external calls and use a reentrancy guard.

[L-4] Potential Reentrancy Attack in TokenDivider::claimNft

Description: External calls to burnFrom and safeTransferFrom are made without reentrancy protection.

Impact: Malicious actors could repeatedly claim NFTs or manipulate state.

Proof of Concept:

ERC20ToGenerateNftFraccion(tokenInfo.erc20Address).burnFrom(msg.sender, erc20ToMintedAmount[tokenInfo.erc20Address]);
IERC721(nftAddress).safeTransferFrom(address(this), msg.sender, tokenInfo.tokenId);

Recommendation: Follow the Checks-Effects-Interactions pattern and use a reentrancy guard

[L-5] Public Functions Not Used Internally Should Be Marked External

Description: Public view functions (getBalanceOf, getErc20TotalMintedAmount, etc.) are not used internally but are marked as public.

Impact: Slightly higher gas costs for external calls.

Recommendation: Change visibility to external:

function getBalanceOf(address user, address token) external view returns(uint256) {
return balances[user][token];
}

[L-6] Naming Convention: Improve Function and Parameter Names

Description: Parameter names (_to, _amount) use an outdated naming convention.

Impact: Reduces code readability and maintainability.

Recommendation: Use modern naming conventions:

function mint(address to, uint256 amount) public onlyOwner {
_mint(to, amount);
}

[L-7] Missing Event Emissions for Critical State Changes

Description: Several critical state changes do not emit events, reducing transparency.

Impact: Harder to track contract activity and debug issues.

Recommendation: Emit events for all significant state changes:

event BalanceUpdated(address indexed user, address indexed erc20, uint256 amount);
emit BalanceUpdated(msg.sender, erc20, amount);

[L-8] Magic Numbers in Fee Calculations in TokenDivider::buyOrder

Description: Magic numbers (e.g., 100, 2) are used in fee calculations, reducing code readability.

Impact: Increases risk of future errors and maintenance challenges.

Recommendation: Replace magic numbers with constants:

uint256 constant FEE_DENOMINATOR = 100;
uint256 constant SELLER_FEE_RATIO = 2;

[L-9] Lack of Event Emission for mint in ERC20ToGenerateNftFraccion::mint

Description: The mint function does not emit an event when tokens are minted.

Impact: Reduces off-chain monitoring capabilities.

Recommendation: Emit an event:

event TokensMinted(address indexed to, uint256 amount);
emit TokensMinted(to, amount);

[L-10] Local Variable Shadowing in ERC20ToGenerateNftFraccion::constructor

Description: Constructor parameters (_name, _symbol) shadow internal variables.

Impact: Reduces code readability and can lead to confusion.

Recommendation: Rename parameters to avoid shadowing:

constructor(string memory name_, string memory symbol_) ERC20(name_, symbol_) {}

Impact

The low-severity findings primarily affect code quality, maintainability, and gas efficiency. While they do not pose immediate risks to funds or protocol functionality, addressing these issues will improve the overall robustness and transparency of the contracts.

Tools Used

Manual Review

Foundry

Recommendations

  1. Remove redundant modifiers and improve code readability.

  2. Follow the Checks-Effects-Interactions pattern to prevent reentrancy.

  3. Mark public functions as external if they are not used internally.

  4. Use modern naming conventions for functions and parameters.

  5. Emit events for all critical state changes to improve transparency.

  6. Replace magic numbers with constants to enhance code maintainability.

  7. Avoid local variable shadowing in constructors.

Updates

Lead Judging Commences

fishy Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

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