052.md

Ambitious Blue Puppy

High

Malicious users can empty tokens from the contract by sending two or more orders in the same block

Summary

Malicious users can empty tokens from the contract by sending two orders in the same block as they will create orders with the same orderId.

Root Cause

The order id is created only by using msg.sender and block.timestamp. [link]

    function generateOrderId(address sender) external view override returns (uint96) {
        uint256 hashedValue = uint256(
            keccak256(abi.encodePacked(sender, block.timestamp))
        );
        return uint96(hashedValue);
    }

This can be used to create 2 or more orders of the same orderId.

First the user calls the createOrder function with a takenIn amount just enough to pass the MASTER.checkMinOrderSize(tokenIn, amountIn); condition. Then user calls createOrder function in the same block with a tokenIn amount which is very high. Since both of these orders will have the same orderId, the later one will replace the first one in the orders mapping. But the orderId's are still duplicated because of this line: pendingOrderIds.push(existingOrderId);

Now the user can call the cancelOrder function to get his funds back. Each time the user cancels an order, the orderid is removed from the pendingOrderIds array. But the corresponding orders mapping is not deleted. This means the below statement in the ordercancel function will return the amount equal to the latest transaction in the block. order.tokenIn.safeTransfer(order.recipient, order.amountIn);

So for example, user first creates an order with tokenIn=100 tokens. then in the same block with tokenIn=10000 tokens.

and calls cancelOrder twice. the contract will return 2*10000 tokens back to the user. This method can be used to empty the contract of any token it has.

The attacker has to just make sure that there are enough tokens balance in the contract.

Mitigation

Create a nonce when calculating order id. Every time a user creates an order, his nonce should be incremented.