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

There is an Incorrect Fee Distribution Logic in the `buyOrder` Function Preventing the Owner from Receiving the Correct Tax Amount

victortheoracle

Summary

The buyOrder function attempts to distribute a fee to the contract owner. However, the code incorrectly assumes that the owner is entitled to the entire fee (fee). Half of the fee (sellerFee) is already sent to the seller, leaving insufficient Ether to transfer the full fee to the owner. This results in the owner not receiving the expected amount.

Vulnerability Details

This is the problematic line of code:

https://github.com/Cyfrin/2025-01-pieces-protocol/blob/4ef5e96fced27334f2a62e388a8a377f97a7f8cb/src/TokenDivider.sol#L298

(bool taxSuccess,) = payable(owner()).call{value: fee}("");

The fee is calculated as order.price / 100, and half of it (sellerFee) is sent to the seller as seen here:

https://github.com/Cyfrin/2025-01-pieces-protocol/blob/4ef5e96fced27334f2a62e388a8a377f97a7f8cb/src/TokenDivider.sol#L292

(bool success, ) = payable(order.seller).call{value: (order.price - sellerFee)}("");

The remaining half is not properly accounted for, leading to an overestimation of the fee available for the owner.

Impact

The contract owner does not receive the correct tax amount, leading to a financial discrepancy and potentially trapping Ether in the contract.

Proof of Concept

To demonstrate this bug, copy this code and paste it in the TokenDividerTest.t.sol file:

function testBuyErc20FeeOverestimated() public nftDivided {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
​
uint256 ownerBalanceBefore = address(tokenDivider.owner()).balance;
uint256 userBalanceBefore = address(USER).balance;
uint256 user2TokenBalanceBefore = tokenDivider.getBalanceOf(USER2, address(erc20Mock));
​
vm.startPrank(USER);
​
erc20Mock.approve(address(tokenDivider), AMOUNT);
​
// Create a sell order for 1 ETH
tokenDivider.sellErc20(address(erc721Mock), 1e18, AMOUNT);
​
uint256 fee = 1e18 / 100; // Fee: 1% of the order price
uint256 sellerFee = fee / 2; // Half of the fee goes to the seller
uint256 expectedOwnerFee = fee - sellerFee;
​
vm.stopPrank();
​
vm.prank(USER2);
​
// The buyer provides enough Ether to cover the price and fee
tokenDivider.buyOrder{value: 1e18 + fee}(0, USER);
​
uint256 ownerBalanceAfter = address(tokenDivider.owner()).balance;
uint256 userBalanceAfter = address(USER).balance;
uint256 user2TokenBalanceAfter = tokenDivider.getBalanceOf(USER2, address(erc20Mock));
​
assertEq(user2TokenBalanceAfter, user2TokenBalanceBefore + AMOUNT, "Buyer should receive the tokens");
assertEq(userBalanceAfter, userBalanceBefore + 1e18 - sellerFee, "Seller should receive their portion");
​
// The owner does not receive the correct fee
assertEq(ownerBalanceAfter - ownerBalanceBefore, expectedOwnerFee, "Owner should receive the remaining fee");
}

Now, run the test with the following command in your terminal:

forge test --mt testBuyErc20FeeOverestimated -vvvv

This test simulates the process of:

  1. Creating a sell order for a specific amount of ERC20 tokens.

  2. Placing a buy order from a different address to purchase those listed tokens.

The point of focus here will be the last assertion:

assertEq(ownerBalanceAfter - ownerBalanceBefore, expectedOwnerFee, "Owner should receive the remaining fee");

This assertion will fail because the owner does not receive the correct fee. The fee is overestimated and exceeds what remains in the contract.

However, if you apply the recommended correction below and rerun the test, the assertion will pass, as the fee distribution will then be accurate.

Tools Used

  • Manual code review

  • Foundry

Recommendations

Adjust the transfer logic for the owner to ensure they receive only the remaining portion of the fee after the seller's share is deducted:

- (bool taxSuccess,) = payable(owner()).call{value: fee}("");
​
+ (bool taxSuccess,) = payable(owner()).call{value: fee - sellerFee}("");
Updates

Lead Judging Commences

fishy Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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