Pieces Protocol

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

Users could loose ETH if amount sent in `TokenDivider::buyOrder()` is greater than orderPrice + sellerFees

Summary

TokenDivider::buyOrder() calculates the sellerFee based on order information. A user is expected to send this amount + order price to complete purchase. The function reverts properly when the amount sent is less that the expected:

if (msg.value < order.price + sellerFee) {
revert TokenDivider__InsuficientEtherForFees();
}

However, this function has no proper handling for when a user sends more eth than required.
When a user sends more eth using this function call, they might loose the extra eth.

Proof of Concept

function testExcessEthIsLostInBuyOrder() public nftDivided {
ERC20Mock erc20Mock = ERC20Mock(
tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address
);
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), AMOUNT);
tokenDivider.sellErc20(address(erc721Mock), 1e18, AMOUNT);
vm.stopPrank();
uint256 price = tokenDivider.getOrderPrice(USER, 0);
assertEq(price, 1e18);
uint256 fees = price / 100;
uint256 sellerFee = fees / 2;
uint256 amtToSend = price + sellerFee;
uint256 user2BalBefore = address(USER2).balance;
vm.startPrank(USER2);
tokenDivider.buyOrder{value: 8e18}(0, USER);
vm.stopPrank();
uint256 user2BalAfter = address(USER2).balance;
assertEq(user2BalAfter, user2BalBefore - 8e18);
console.log(user2BalBefore);
console.log(user2BalAfter);
}

Impact

User could loose Eth

Tools Used

Manual review

Recommendations

In buyOrder function, add a mechanism to check for when the amount transfered is more.

Updates

Lead Judging Commences

fishy Lead Judge 8 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.