Pieces Protocol

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

If buyer pay more ETH, the extra could become locked in the contract, leading to potential fund loss.

Description:

in the buyOrder function, the contract does not check
if the buyer sends more ETH than the order price,
and it will only transfer the 2% fee to the seller.
if more ETH is sent, the extra will be locked in the contract

function buyOrder(uint256 orderIndex, address seller) external payable {
...
@> (bool taxSuccess, ) = payable(owner()).call{value: fee}("");
...
}

Impact:

Loss of Funds

Proof of Concept:

add the following in test/unit/TokenDividerTest.t.sol

modifier nftSell(uint256 price, uint256 sellAmount) {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), sellAmount);
tokenDivider.sellErc20(address(erc721Mock), price, sellAmount);
vm.stopPrank();
_;
}
...
function testFundLocked() public nftDivided nftSell(1e18, AMOUNT){
uint256 price = 1e18;
uint256 fee = price / 100;
uint256 sellerFee = fee / 2;
uint256 extra = price / 2;
uint256 sentValue = price + sellerFee + extra;
uint256 index = 0;
vm.startPrank(USER2);
tokenDivider.buyOrder{value: sentValue}(index, USER);
vm.stopPrank();
assert(address(tokenDivider).balance == extra);
}

Recommended Mitigation:

Add a withdrawal function instead of directly transferring the ETH to the owner of the contract

+ function withdrawETH() external onlyOwner {
+ require(address(this).balance > 0, "No funds to withdraw");
+ payable(owner()).transfer(address(this).balance);
+ }
Updates

Lead Judging Commences

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