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

Sell order of a user moves to a different index in the array after successful buyOrder

joesepherus

Vulnerability Details

The TokenDivider::buyOrder function removes the sell order at orderIndex and moves to its place(index) a sell order from the end of the s_userToSellOrders array. This might confused a buyer who wants to buy order at index 0, but anoter buyer has already bought the order at index 0 and the first buyer will now buy an order he didn't intend to, buying a different nft from the one he wanted. Another problem is there is no way to get data about the sell order, only thing you can query is the price using TokenDivider:getOrderPrice function. Buyers are left in the dark, they have no idea what they are even buying, what erc tokens to which nft.

Impact

Buyer buying a different nft from the one he wanted. This creates confusion for the buyers/users of the contract.

Proof of Concept:

  1. USER mints 3 diffrent nfts and approves and divides them

  2. USER creates sell orders for all 3 nfts

  3. USER2 buys order at index 0, now the sell order at index 2 replaces the sell order at index 0

PoC Code

Add following test:

​
function testSellOrderMovesToDifferentIndex() public nftDivided {
// mint, approve and divide 2 more nfts with different addresses, names, symbols
ERC721Mock erc721Mock2 = new ERC721Mock("mock1", "mck1");
erc721Mock2.mint(USER);
​
ERC721Mock erc721Mock3 = new ERC721Mock("mock2", "mck2");
erc721Mock3.mint(USER);
​
vm.startPrank(USER);
erc721Mock2.approve(address(tokenDivider), 0);
tokenDivider.divideNft(address(erc721Mock2), 0, AMOUNT);
​
erc721Mock3.approve(address(tokenDivider), 0);
tokenDivider.divideNft(address(erc721Mock3), 0, AMOUNT);
vm.stopPrank();
​
// get the erc20 tokens related to the nfts
ERC20Mock erc20Mock = ERC20Mock(
tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address
);
ERC20Mock erc20Mock2 = ERC20Mock(
tokenDivider.getErc20InfoFromNft(address(erc721Mock2)).erc20Address
);
ERC20Mock erc20Mock3 = ERC20Mock(
tokenDivider.getErc20InfoFromNft(address(erc721Mock3)).erc20Address
);
​
// setup sell orders for the nfts
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), AMOUNT);
erc20Mock2.approve(address(tokenDivider), AMOUNT);
erc20Mock3.approve(address(tokenDivider), AMOUNT);
tokenDivider.sellErc20(address(erc721Mock), 1, AMOUNT);
tokenDivider.sellErc20(address(erc721Mock2), 2, AMOUNT);
tokenDivider.sellErc20(address(erc721Mock3), 3, AMOUNT);
vm.stopPrank();
​
// buy the nft at index 0
vm.prank(USER2);
tokenDivider.buyOrder{value: (3e18)}(0, USER);
​
// now the last one(the one in index 2) goes to first place in the array
// what if you want to buy the one at index 0 but end up buing the one at
// index 2 because someone already bought the one at index 0 before you
// you'll be paying and buying nft that you didn't want in the first place
uint256 price = tokenDivider.getOrderPrice(USER, 0);
assertEq(price, 3);
}

Tools Used

Manual review

Recommendations

To prevent this, don't manipulate the s_userToSellOrders array in such a way, use unique identifiers for sell orders. Also add some more getters for the sell orders so it's easier for buyers to query what they might want to buy.

Updates

Lead Judging Commences

fishy Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

joesepherus Submitter
5 months ago
fishy Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Front-running

Support

FAQs

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