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

unchecked-transfer in TokenDivider.sol

jk_sca

Summary

The buyOrder function in the TokenDivider contract fails to handle token transfer failures adequately. When the transfer call in the ERC20 contract fails, the transaction does not fully revert. This results in partially completed operations, leaving user funds in an inconsistent state and tokens locked in the contract.

Vulnerability Details

​
​
TokenDivider.buyOrder(uint256,address)](src/TokenDivider.sol#L261-L305) ignores return value by [IERC20(order.erc20Address).transfer(msg.sender,order.amount)]()
​
src/TokenDivider.sol#L261-L305
​

No check of transfer success

Transaction continues even if transfer fails

PoC

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.18;
​
import {Test} from "forge-std/Test.sol";
import {TokenDivider} from "../src/TokenDivider.sol";
import {MockNFT} from "./mocks/MockNFT.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​
contract TokenDividerTest is Test {
TokenDivider public tokenDivider;
MockNFT public mockNft;
address public fracTokenAddress;
address public user1 = makeAddr("user1");
address public user2 = makeAddr("user2");
uint256 constant SELL_PRICE = 1 ether;
uint256 constant SELL_AMOUNT = 100;
receive() external payable {}
​
function setUp() public {
tokenDivider = new TokenDivider();
mockNft = new MockNFT();
vm.startPrank(user1);
mockNft.mint(user1, 1);
mockNft.approve(address(tokenDivider), 1);
tokenDivider.divideNft(address(mockNft), 1, 1000);
fracTokenAddress = tokenDivider.getErc20InfoFromNft(address(mockNft)).erc20Address;
IERC20(fracTokenAddress).approve(address(tokenDivider), type(uint256).max);
vm.stopPrank();
​
// Give test contract ETH to accept fees
vm.deal(address(this), 10 ether);
}
​
function testBuyOrderVulnerability() public {
uint256 initialSellerBalance = user1.balance;
// User1 creates sell order
vm.prank(user1);
tokenDivider.sellErc20(address(mockNft), SELL_PRICE, SELL_AMOUNT);
​
emit log("\n=== Before Buy Order ===");
emit log_named_uint("User1 ETH Balance", user1.balance);
emit log_named_uint("User1 Token Balance", IERC20(fracTokenAddress).balanceOf(user1));
emit log_named_uint("User2 Token Balance", IERC20(fracTokenAddress).balanceOf(user2));
emit log_named_uint("Contract Token Balance", IERC20(fracTokenAddress).balanceOf(address(tokenDivider)));
​
// User2 attempts to buy
vm.startPrank(user2);
vm.deal(user2, 2 ether);
// Mock token transfer to fail
vm.mockCall(
fracTokenAddress,
abi.encodeWithSelector(IERC20.transfer.selector, user2, SELL_AMOUNT),
abi.encode(false)
);
​
tokenDivider.buyOrder{value: SELL_PRICE + 0.02 ether}(0, user1);
​
emit log("\n=== After Buy Order ===");
emit log_named_uint("User1 ETH Received", user1.balance - initialSellerBalance);
emit log_named_uint("User1 Token Balance", IERC20(fracTokenAddress).balanceOf(user1));
emit log_named_uint("User2 Token Balance", IERC20(fracTokenAddress).balanceOf(user2));
emit log_named_uint("Contract Token Balance", IERC20(fracTokenAddress).balanceOf(address(tokenDivider)));
vm.stopPrank();
}
}

Results:

forge test -vvv --match-test testBuyOrderVulnerability -vvv
[⠰] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/TokenDivider.t.sol:TokenDividerTest
[PASS] testBuyOrderVulnerability() (gas: 224068)
Logs:
​
=== Before Buy Order ===
User1 ETH Balance: 0
User1 Token Balance: 900
User2 Token Balance: 0
Contract Token Balance: 100
​
=== After Buy Order ===
User1 ETH Received: 995000000000000000
User1 Token Balance: 900
User2 Token Balance: 0
Contract Token Balance: 100
​
Traces:
[3570733] TokenDividerTest::setUp()
├─ [1819251] → new TokenDivider@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: TokenDividerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return] 8968 bytes of code
├─ [816931] → new MockNFT@0x2e234DAe75C793f67A35089C9d99245E1C58470b
│ └─ ← [Return] 3855 bytes of code
├─ [0] VM::startPrank(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Return]
├─ [47187] MockNFT::mint(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 1)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], tokenId: 1)
│ └─ ← [Stop]
├─ [25046] MockNFT::approve(TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ ├─ emit Approval(owner: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], approved: TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], tokenId: 1)
│ └─ ← [Stop]
├─ [711076] TokenDivider::divideNft(MockNFT: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1, 1000)
│ ├─ [572] MockNFT::ownerOf(1) [staticcall]
│ │ └─ ← [Return] user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]
│ ├─ [572] MockNFT::ownerOf(1) [staticcall]
│ │ └─ ← [Return] user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]
│ ├─ [1174] MockNFT::name() [staticcall]
│ │ └─ ← [Return] "MockNFT"
│ ├─ [1196] MockNFT::symbol() [staticcall]
│ │ └─ ← [Return] "MNFT"
│ ├─ [452565] → new ERC20ToGenerateNftFraccion@0x104fBc016F4bb334D775a19E8A6510109AC63E00
│ │ └─ ← [Return] 2031 bytes of code
│ ├─ [46894] ERC20ToGenerateNftFraccion::mint(TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1000)
│ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1000)
│ │ └─ ← [Stop]
│ ├─ [30860] MockNFT::safeTransferFrom(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1, 0x)
│ │ ├─ emit Transfer(from: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], to: TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], tokenId: 1)
│ │ ├─ [664] TokenDivider::onERC721Received(TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 1, 0x)
│ │ │ └─ ← [Return] 0x150b7a02
│ │ └─ ← [Stop]
│ ├─ [572] MockNFT::ownerOf(1) [staticcall]
│ │ └─ ← [Return] TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]
│ ├─ emit NftDivided(nftAddress: MockNFT: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], amountErc20Minted: 1000, erc20Minted: ERC20ToGenerateNftFraccion: [0x104fBc016F4bb334D775a19E8A6510109AC63E00])
│ ├─ [25203] ERC20ToGenerateNftFraccion::transfer(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 1000)
│ │ ├─ emit Transfer(from: TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], value: 1000)
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [854] TokenDivider::getErc20InfoFromNft(MockNFT: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
│ └─ ← [Return] ERC20Info({ erc20Address: 0x104fBc016F4bb334D775a19E8A6510109AC63E00, tokenId: 1 })
├─ [24734] ERC20ToGenerateNftFraccion::approve(TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ ├─ emit Approval(owner: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], spender: TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ └─ ← [Return] true
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::deal(TokenDividerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 10000000000000000000 [1e19])
│ └─ ← [Return]
└─ ← [Stop]
​
[285351] TokenDividerTest::testBuyOrderVulnerability()
├─ [0] VM::prank(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Return]
├─ [158617] TokenDivider::sellErc20(MockNFT: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1000000000000000000 [1e18], 100)
│ ├─ emit OrderPublished(amount: 100, seller: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], nftPegged: MockNFT: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
│ ├─ [32420] ERC20ToGenerateNftFraccion::transferFrom(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 100)
│ │ ├─ emit Transfer(from: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], to: TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 100)
│ │ ├─ storage changes:
│ │ │ @ 0x266f7372a76c4363ea04e26dfd6a2a19d44ec1cadb0d91060c1fb98ef6adc19b: 1000 → 900
│ │ │ @ 0x996b860ef646da948c7d745162e3851398cba818db7bc8c7a4d9d465a54dfa11: 0 → 100
│ │ └─ ← [Return] true
│ ├─ storage changes:
│ │ @ 0x89f4dda5c819fbddd16bb9ad04ae6f4f53e9598fdcd85a64e2934551e1ca4055: 0 → 100
│ │ @ 0xffa349e12903d55d44b03963e778e0466a588c951f33196e2aad9d758af52678: 0 → 1
│ │ @ 0x3e35f3d712e6159836b1bf1ead524757949cb2df7e6fe5147bd05f40e1049396: 1000 → 900
│ │ @ 0x89f4dda5c819fbddd16bb9ad04ae6f4f53e9598fdcd85a64e2934551e1ca4052: 0 → 0x00000000000000000000000029e3b139f4393adda86303fcdaa35f60bb7092bf
│ │ @ 0x89f4dda5c819fbddd16bb9ad04ae6f4f53e9598fdcd85a64e2934551e1ca4053: 0 → 0x000000000000000000000000104fbc016f4bb334d775a19e8a6510109ac63e00
│ │ @ 0x89f4dda5c819fbddd16bb9ad04ae6f4f53e9598fdcd85a64e2934551e1ca4054: 0 → 0x0000000000000000000000000000000000000000000000000de0b6b3a7640000
│ └─ ← [Stop]
├─ emit log(val: "\n=== Before Buy Order ===")
├─ emit log_named_uint(key: "User1 ETH Balance", val: 0)
├─ [559] ERC20ToGenerateNftFraccion::balanceOf(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 900
├─ emit log_named_uint(key: "User1 Token Balance", val: 900)
├─ [2559] ERC20ToGenerateNftFraccion::balanceOf(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]) [staticcall]
│ └─ ← [Return] 0
├─ emit log_named_uint(key: "User2 Token Balance", val: 0)
├─ [559] ERC20ToGenerateNftFraccion::balanceOf(TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
│ └─ ← [Return] 100
├─ emit log_named_uint(key: "Contract Token Balance", val: 100)
├─ [0] VM::startPrank(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Return]
├─ [0] VM::deal(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], 2000000000000000000 [2e18])
│ └─ ← [Return]
├─ [0] VM::mockCall(ERC20ToGenerateNftFraccion: [0x104fBc016F4bb334D775a19E8A6510109AC63E00], 0xa9059cbb000000000000000000000000537c8f3d3e18df5517a58b3fb9d91436979968020000000000000000000000000000000000000000000000000000000000000064, 0x0000000000000000000000000000000000000000000000000000000000000000)
│ └─ ← [Return]
├─ [69920] TokenDivider::buyOrder{value: 1020000000000000000}(0, user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ ├─ emit OrderSelled(buyer: user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], price: 1000000000000000000 [1e18])
│ ├─ [0] user1::fallback{value: 995000000000000000}()
│ │ └─ ← [Stop]
│ ├─ [55] TokenDividerTest::receive{value: 10000000000000000}()
│ │ └─ ← [Stop]
│ ├─ [0] ERC20ToGenerateNftFraccion::transfer(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], 100)
│ │ └─ ← [Return] false
│ ├─ storage changes:
│ │ @ 0xffa349e12903d55d44b03963e778e0466a588c951f33196e2aad9d758af52678: 1 → 0
│ │ @ 0x89f4dda5c819fbddd16bb9ad04ae6f4f53e9598fdcd85a64e2934551e1ca4053: 0x000000000000000000000000104fbc016f4bb334d775a19e8a6510109ac63e00 → 0
│ │ @ 0x89f4dda5c819fbddd16bb9ad04ae6f4f53e9598fdcd85a64e2934551e1ca4052: 0x00000000000000000000000029e3b139f4393adda86303fcdaa35f60bb7092bf → 0
│ │ @ 0x916164bb747da7612753fdea4750bf6dffb9d8ce7252274385dd43052cf26491: 0 → 100
│ │ @ 0x89f4dda5c819fbddd16bb9ad04ae6f4f53e9598fdcd85a64e2934551e1ca4054: 0x0000000000000000000000000000000000000000000000000de0b6b3a7640000 → 0
│ │ @ 0x89f4dda5c819fbddd16bb9ad04ae6f4f53e9598fdcd85a64e2934551e1ca4055: 100 → 0
│ └─ ← [Stop]
├─ emit log(val: "\n=== After Buy Order ===")
├─ emit log_named_uint(key: "User1 ETH Received", val: 995000000000000000 [9.95e17])
├─ [559] ERC20ToGenerateNftFraccion::balanceOf(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 900
├─ emit log_named_uint(key: "User1 Token Balance", val: 900)
├─ [559] ERC20ToGenerateNftFraccion::balanceOf(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]) [staticcall]
│ └─ ← [Return] 0
├─ emit log_named_uint(key: "User2 Token Balance", val: 0)
├─ [559] ERC20ToGenerateNftFraccion::balanceOf(TokenDivider: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
│ └─ ← [Return] 100
├─ emit log_named_uint(key: "Contract Token Balance", val: 100)
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ storage changes:
│ @ 0x996b860ef646da948c7d745162e3851398cba818db7bc8c7a4d9d465a54dfa11: 0 → 100
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.28ms (778.50µs CPU time)
​
Ran 1 test suite in 1.24s (1.28ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Impact

High financial risk

Direct potential loss of funds

Breaks expected transaction atomicity

Tools Used

Foundry test suite

Manual code review

Recommendations

function buyOrder(uint256 orderIndex, address seller) external payable {
// Existing checks...
​
// Add explicit transfer check
require(
IERC20(order.erc20Address).transfer(msg.sender, order.amount),
"Token transfer failed"
);
}

Implement mandatory token transfer verification to ensure buyer receives tokens before finalizing transaction.

Updates

Lead Judging Commences

fishy Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Other

Appeal created

jk_sca Submitter
9 months ago
jk_sca Submitter
9 months ago
fishy Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Token misshandling

The extra eth sent by the user in the buy order will be locked in the contract forever

Support

FAQs

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