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

The `TokenDivider` contract uses the `balances` mapping for internal accounting instead of default ERC20 balanceOf() function, which may lead to incorrect token balances retrieved.

xilobytes

Description

The contract implements mapping(address user => mapping(address erc20Address => uint256 amount)) balances for internal accounting of minting, burning, and transfer of ERC20 tokens created when dividing an NFT.

Impact

Users can transfer ERC20 tokens using the default .transfer() function, instead of using the TokenDivider::transferErcTokens. This will lead to the TokenDivider not having the updated and actual balance of tokens for the users.

Proof of Concepts

Add the following test to TokenDividerTest.t.sol file:

function testInternalBalancesAccountingLeadsToWrongResults() public {
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
​
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
erc20Mock.transfer(USER2, AMOUNT);
vm.stopPrank();
​
uint256 userBalance = tokenDivider.getBalanceOf(USER, address(erc20Mock));
uint256 user2Balance = tokenDivider.getBalanceOf(USER2, address(erc20Mock));
​
// The test will fail
assertEq(erc20Mock.balanceOf(USER), userBalance);
assertEq(erc20Mock.balanceOf(USER2), user2Balance);
}

Tools Used

  • Foundry, Manual analysis

Recommended mitigation

Remove the balances mapping and implement the ERC20 balanceOf() default function instead.

Updates

Lead Judging Commences

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

Transfer ERC20ToGenerateNftFraccion separately to the contract

Support

FAQs

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