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

[H-2] The `TokenDivider::buyOrder` function can be front-runned and the user can buy a wrong `TokenDivider::SellOrder`

pindarew

Description buyOrder gets called by the seller address and index of seller orders. SaleOrder struct looks like this:

struct SellOrder {
address seller;
address erc20Address;
uint256 price;
uint256 amount;
}

So when a good offer is listed by TokenDivider::sellErc20, there can be more than one user interested in it. So the one sending the transaction faster and ready to spend more on gas will buy the order faster. This is fair, but it creates a vulnerability for other users because when their transaction comes, the order of sell orders from mapping(address user => SellOrder[] orders) s_userToSellOrders is now changed.
But protocol can't know if there is a problem with that, so check if there is an order at that index from that seller, and that's it. However, a user will get a wrong offer, maybe more expensive, maybe less ERC20s, and maybe a wrong ERC20 that you don't need.

Impact User will buy the wrong offer, with the wrong ERC20 tokens, or the offer can be more expensive. Also, he can pay the same amount of money for less ERC20 tokens. When more users arrive in the protocol the impact will be a lot bigger, because will happen on a daily base.

Proof of Concepts

  1. Seller creates an orders to sell for example:

    1. Index 0 = SellOrder1 {seller, erc20Address, price: 2e18, amount: 2e18}

    2. Index 1 = SellOrder2 {seller, erc20Address, price: 2e18, amount: 1e18}

  2. At this point every user will prefer the order at index zero, so they will try to buy it

  3. We have two users who want to buy the same offer, and they send their transaction at around the same time, but one of the guys set higher prices for gas

  4. So the guy who spent more gas bought the order

  5. Now orders look like this:

    1. Index 0 = SellOrder2 {seller, erc20Address, price: 2e18, amount: 1e18}

  6. The expected behavior for other user's transaction is to revert, but instead, we buy the other offer, that we didn't want and spent the same price for fewer tokens

Change this modifier from test file TokenDividerTest.t.sol

modifier nftDivided() {
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
- tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
+ tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, 4e18);
vm.stopPrank();
​
_;
}

Place the following code in the test file TokenDividerTest.t.sol

function testBuyOrderFrontrunning() public nftDivided {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
address attackerFrontrunner = makeAddr("attackerFrontrunner");
deal(attackerFrontrunner, STARTING_USER_BALANCE);
​
vm.startPrank(USER);
​
erc20Mock.approve(address(tokenDivider), 3e18);
tokenDivider.sellErc20(address(erc721Mock), 2e18, 2e18); // s_userToSellOrders[USER][0]
tokenDivider.sellErc20(address(erc721Mock), 2e18, 1e18); // s_userToSellOrders[USER][1]
​
vm.stopPrank();
​
​
uint256 fees = 2e18 / 100;
​
vm.prank(attackerFrontrunner);
tokenDivider.buyOrder{value: (2e18 + fees)}(0, USER);
​
vm.prank(USER2);
tokenDivider.buyOrder{value: (2e18 + fees)}(0, USER);
​
assert(erc20Mock.balanceOf(USER2) < 2e18);
}

And run the following command in the terminal forge test --mt testBuyOrderFrontrunning -vvvv

Recommended mitigation Consider adding some checks of what a user expects to retrieve at least what is the ERC20 address of tokens he wants, maybe even what is the price and amount. The user will be sure that he gets the offer he wants.

Example:

contract TokenDivider is IERC721Receiver, Ownable {
.
.
.
error TokenDivider__IncorrectEtherAmount();
error TokenDivider__InsuficientEtherForFees();
+ error TokenDivier__InvalidOrder();
​
function buyOrder(
uint256 orderIndex,
address seller
+ , address erc20,
+ uint256 amount,
+ uint256 price
) external payable {
if (seller == address(0)) {
revert TokenDivider__InvalidSeller();
}
​
SellOrder memory order = s_userToSellOrders[seller][orderIndex];
​
+ if (order.amount != amount || order.price != price || order.erc20Address != erc20) {
+ revert TokenDivier__InvalidOrder();
+ }
.
.
.
}

!Note! After making changes of functions make sure to check you scripts and update it!

Updates

Lead Judging Commences

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.