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

In the `TokenDivider::buyOrder` function, the shifting in the sell orders array of `s_userToSellOrders` mapping can result in users unintentionally purchasing the wrong order due to index mismatch.

xilobytes

Description

The TokenDivider::buyOrder function manages sell orders using an array (s_userToSellOrders). When an order is fulfilled, the function replaces the specified order with the last order in the array and shortens the array using the pop() operation. This design introduces a race condition: if multiple users attempt to buy the same order index in rapid succession, the order at that index may change due to the shifting of array elements. Consequently, subsequent buyers may unintentionally purchase the wrong order or experience transaction failures if the new order's price or conditions differ from their expectations.

Impact

There are many different impacts that this issue may cause:

  1. Buyers may unknowingly purchase an order different from the one they intended, leading to confusion and a poor user experience.

  2. If a subsequent buyer sends ETH matching the original order but insufficient for the new order, the transaction will revert, causing inconvenience and wasted gas fees.

Proof of Concepts

Add the following test to TokenDividerTest.t.sol file:

function testBuyOrderRaceCondition() public nftDivided {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
​
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), AMOUNT);
tokenDivider.sellErc20(address(erc721Mock), 1e18, AMOUNT / 4); // Order 0
tokenDivider.sellErc20(address(erc721Mock), 2e18, AMOUNT / 4); // Order 1
tokenDivider.sellErc20(address(erc721Mock), 3e18, AMOUNT / 4); // Order 2
tokenDivider.sellErc20(address(erc721Mock), 4e18, AMOUNT / 4); // Order 3
vm.stopPrank();
​
uint256 user2InitialBalance = USER2.balance;
uint256 user3InitialBalance = STARTING_USER_BALANCE;
uint256 sellerInitialBalance = USER.balance;
​
​
vm.startPrank(USER2);
vm.deal(USER2, STARTING_USER_BALANCE);
tokenDivider.buyOrder{value: 2e18}(1, USER);
vm.stopPrank();
​
// Another user (USER3) tries to buy Order 1 but will end up buying Order 3 due to shifting
address USER3 = makeAddr("user3");
vm.startPrank(USER3);
vm.deal(USER3, STARTING_USER_BALANCE);
tokenDivider.buyOrder{value: 4e18}(1, USER); // Order 1 is now replaced by Order 3
vm.stopPrank();
​
// Assert
uint256 user2FinalBalance = USER2.balance;
uint256 user3FinalBalance = USER3.balance;
uint256 sellerFinalBalance = USER.balance;
​
uint256 user3Tokens = tokenDivider.getBalanceOf(USER3, address(erc20Mock));
uint256 user2Tokens = tokenDivider.getBalanceOf(USER2, address(erc20Mock));
​
// User 2 should have received the tokens for Order 1
assertEq(user2Tokens, AMOUNT / 4, "USER2 did not receive tokens for Order 1");
​
// User 3 should have unintentionally received the tokens for Order 3
assertEq(user3Tokens, AMOUNT / 4, "USER3 did not receive tokens for Order 3");
​
assertEq( user2InitialBalance - user2FinalBalance, 2e18);
// USER3 ends up spending 4 ETH when they intended to buy Order 1 which initially costed 2 ETH (before the shifting)
assertEq(user3InitialBalance - user3FinalBalance, 4e18);
}

Tools Used

  • Foundry, Manual analysis

Recommended mitigation

Add the following change to the contract:

​
+ uint256 public nextOrderId;
mapping(address user => mapping(address erc20Address => uint256 amount)) balances;
mapping(address nft => ERC20Info) nftToErc20Info;
- mapping(address user => SellOrder[] orders) s_userToSellOrders;
+ mapping(uint256 orderId => SellOrder) public sellOrders;
+ mapping(uint256 orderId => address seller) public orderOwners;
mapping(address erc20 => address nft) erc20ToNft;
mapping(address erc20 => uint256 totalErc20Minted) erc20ToMintedAmount;
​
.
.
.
​
function sellErc20(address nftPegged, uint256 price,uint256 amount) external {
if(nftPegged == address(0)) {
revert TokenDivider__NftAddressIsZero();
}
​
if( amount == 0) {
revert TokenDivider__AmountCantBeZero();
}
ERC20Info memory tokenInfo = nftToErc20Info[nftPegged];
if(balances[msg.sender][tokenInfo.erc20Address] < amount) {
revert TokenDivider__InsuficientBalance();
}
​
balances[msg.sender][tokenInfo.erc20Address] -= amount;
​
- s_userToSellOrders[msg.sender].push(
- SellOrder({
- seller: msg.sender,
- erc20Address: tokenInfo.erc20Address,
- price: price,
- amount: amount
- })
- );
​
+ uint256 orderId = nextOrderId++;
+ sellOrders[orderId] = SellOrder({
+ seller: msg.sender,
+ erc20Address: tokenInfo.erc20Address,
+ price: price,
+ amount: amount
+ });
+ orderOwners[orderId] = msg.sender;
​
emit OrderPublished(amount,msg.sender, nftPegged);
​
IERC20(tokenInfo.erc20Address).transferFrom(msg.sender,address(this), amount);
}
​
- function buyOrder(uint256 orderIndex, address seller) external payable {
+ function buyOrder(uint256 orderId, address seller) external payable {
if(seller == address(0)) {
revert TokenDivider__InvalidSeller();
}
​
- SellOrder memory order = s_userToSellOrders[seller][orderIndex];
+ SellOrder memory order = sellOrders[orderId];
​
if(msg.value < order.price) {
revert TokenDivider__IncorrectEtherAmount();
}
​
uint256 fee = order.price / 100;
uint256 sellerFee = fee / 2;
​
​
if(msg.value < order.price + sellerFee) {
revert TokenDivider__InsuficientEtherForFees();
}
​
balances[msg.sender][order.erc20Address] += order.amount;
​
- s_userToSellOrders[seller][orderIndex] = s_userToSellOrders[seller][s_userToSellOrders[seller].length - 1];
- s_userToSellOrders[seller].pop();
+ delete sellOrders[orderId];
+ delete orderOwners[orderId];
emit OrderSelled(msg.sender, order.price);
// Transfer The Ether
​
(bool success, ) = payable(order.seller).call{value: (order.price - sellerFee)}("");
​
if(!success) {
revert TokenDivider__TransferFailed();
}
​
(bool taxSuccess, ) = payable(owner()).call{value: fee}("");
​
​
if(!taxSuccess) {
revert TokenDivider__TransferFailed();
}
​
IERC20(order.erc20Address).transfer(msg.sender, order.amount);
​
}
​
Updates

Lead Judging Commences

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

Appeal created

xilobytes Submitter
8 months ago
fishy Lead Judge 8 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.