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);
+ }