Skip to content

Latest commit

 

History

History
229 lines (180 loc) · 10.8 KB

062.md

File metadata and controls

229 lines (180 loc) · 10.8 KB

Damp Fuchsia Buffalo

High

Malicious user can grief legitimate users by permanently locking their pre-approved ERC20 funds in the StopLimit contract

Summary

There're multiple problems with the code from which the vulnerability eventually arised:

  1. No access control on the createOrder method in StopLimit --- whereas there should be either a check that msg.sender == (the specified) recipient, OR that the msg.sender is charged with transferFrom specifically (not the specified recipient).
  2. AutomationMaster::generateOrderId is assumed to be a function that generates unique order IDs. However, it solely derives the order ID based on the msg.sender and the block.timestamp, without even encoding any salt or nonce value along with it. For any consecutive transaction made within the block N by the EOA or smart contract "0xabc", the order ID will always be the same.
  3. StopLimit::createOrder passes the msg.sender as the "sender" to the AutomationMaster's generateOrderId function. This means that a given caller will always produce the same order ID, even with different recipients specified, as long as he createsOrder within a single block, so that the block.timestamp remains the same.

Root Cause

    function _createOrder(
        uint256 stopLimitPrice,
        uint256 takeProfit,
        uint256 stopPrice,
        uint256 amountIn,
        IERC20 tokenIn,
        IERC20 tokenOut,
        address recipient,
        uint16 feeBips,
        uint16 takeProfitSlippage,
        uint16 stopSlippage,
        uint16 swapSlippage,
        bool swapOnFill
    ) internal {
        //verify both oracles exist, as we need both to calc the exchange rate
        require(
            address(MASTER.oracles(tokenIn)) != address(0x0) &&
                address(MASTER.oracles(tokenOut)) != address(0x0),
            "Oracle !exist"
        );
        require(
            pendingOrderIds.length < MASTER.maxPendingOrders(),
            "Max Order Count Reached"
        );
        require(
            takeProfitSlippage <= 10000 &&
                stopSlippage <= 10000 &&
                swapSlippage <= 10000 &&
                feeBips <= 10000,
            "BIPS > 10k"
        );

        MASTER.checkMinOrderSize(tokenIn, amountIn);

        uint96 orderId = MASTER.generateOrderId(msg.sender);

        orders[orderId] = Order({
            orderId: orderId,
            stopLimitPrice: stopLimitPrice,
            stopPrice: stopPrice,
            takeProfit: takeProfit,
            amountIn: amountIn,
            tokenIn: tokenIn,
            tokenOut: tokenOut,
            takeProfitSlippage: takeProfitSlippage,
            feeBips: feeBips,
            stopSlippage: stopSlippage,
            swapSlippage: swapSlippage,
            recipient: recipient,
            direction: MASTER.getExchangeRate(tokenIn, tokenOut) >
                stopLimitPrice, //compare to stop price for this order's direction
            swapOnFill: swapOnFill
        });
        pendingOrderIds.push(uint96(orderId));
        //emit
        emit OrderCreated(orderId);
    }

Moreover, the approach to store orders is pathological too.

The orders[orderId] mapping entry is never cleared even after cancelOrder is called.

cancelOrder will only remove 1 item from the pendingOrderIds array: specifically, the 1st found item in that array, the orderId of which is the target one (orderId):

    function _cancelOrder(uint96 orderId) internal returns (bool) {
        Order memory order = orders[orderId];
        for (uint96 i = 0; i < pendingOrderIds.length; i++) {
            if (pendingOrderIds[i] == orderId) {
                //remove from pending array
                pendingOrderIds = ArrayMutation.removeFromArray(
                    i,
                    pendingOrderIds
                );
                order.tokenIn.safeTransfer(order.recipient, order.amountIn);

                //emit event
                emit OrderCancelled(orderId);

                //short circuit loop
                return true;
            }
        }
        return false;
    }

As you can see, generateOrderId is implemented incorrectly, and it will return colliding order IDs very often!

    ///@notice generate a random and unique order id
    function generateOrderId(address sender) external view override returns (uint96) {
        uint256 hashedValue = uint256(
            keccak256(abi.encodePacked(sender, block.timestamp))
        );
        return uint96(hashedValue);
    }

Internal pre-conditions

The users are expected to grant StopLimit ERC20 approvals prior to calling createOrder.

Some users may even grant "infinite" approvals <--- this is not an explicit prerequisite for this attack though! Any approval is fine, as long as it is just there.

External pre-conditions

No response

Attack Path

  1. Alice, Stella and Marie have granted ERC20 approvals to the StopLimit contract --- they all want to initialize orders and use Oku.
  2. Malicious "Ninja" detects that the StopLimit contract has unconsumed ERC20 approvals.
  3. First, Ninja calls createOrder, specifying the recipient as Alice, and the amountIn as the whole amount that Alice has pre-approved the contract for. This generates an order ID as uint96(uint256(keccak256(abi.encodePacked(sender, block.timestamp)))). The sender here is the Ninja's own address. (Ninja will probably be a smart contract!). The funds are transferred from Alice to the StopLimit contract, as she gave prior approval to the StopLimit contract before. A new item is pushed to the array, consisting solely of the generated orderId: pendingOrderIds.push(uint96(orderId));.
  4. Notice this is all done in a batch transaction manner, i.e. all within the same block.
  5. Ninja then calls createOrder within the very same block (probably in scheduling transactions in a batch manner, atomically); he specifies the recipient as Stella, and drains the whole pre-approved tokenIn amountIn that Stella has granted to the StopLimit contract. The order for the recipient =Stella is recorded based on the same orderID (due to the flawed logic and implementation in StopLimit::generateOrderId). A new item with the same orderId is pushed to the array: pendingOrderIds.push(uint96(orderId));.
  6. Next, Ninja proceeds with griefing Marie by creating an order via StopLimit::createOrder, specifying the recipient=Marie, the amountIn as the maximum amount of tokenIn that Marie has approved to the StopLimit contract (when she wanted to legitimately create an order!). The orders[orderId] entry is overriden. Now, due to a colliding, pseudo-unique (quasi-unique!!) order ID, the orders[orderId] item neither points to Alice's nor Stella's created and prefunded orders --- it points to the Marie's order only! A new item is pushed to the pendingOrderIds array, though its value is still the same, old orderId that was generated the same way as Alice's and Stella's order IDs were, when malicious Ninja approached this attack.
  7. Finally, Ninja calls StopLimit::createOrder; and he creates an order with recipient=Ninja and tokenIn and amountIn values of his choice.

The Alice's, Stella's and Marie's funded orders will now be forever lost.

Moreover, Ninja not only griefed these victims through this attack, but he can also get real monetary profit from this now.

As there're 4 items in pendingOrderIds that point to the very same, identical orderId, Ninja can call StopLimit::cancelOrder 4 times, and each time he will get refunds for the 4th order he created.

The check msg.sender == order.recipient will be 100% fulfilled, because at the end orders[orderId] was overriden with an order, the params of which were of Ninja's choice and the recipient was he himself (recipient=Ninja).

He will get 4x the refund of the 4rth order amountIn!

    ///@notice only the order recipient can cancel their order
    ///@notice only pending orders can be cancelled
    function cancelOrder(uint96 orderId) external {
        Order memory order = orders[orderId];
        require(msg.sender == order.recipient, "Only Order Owner");
        require(_cancelOrder(orderId), "Order not active");
    }

The ArrayMutation.removeFromArray function will go through smoothly, as it can find 4 items where indeed orderId will be the "target" orderId that Ninja specified --- they're all identical anyway!

    function _cancelOrder(uint96 orderId) internal returns (bool) {
        Order memory order = orders[orderId];
        for (uint96 i = 0; i < pendingOrderIds.length; i++) {
            if (pendingOrderIds[i] == orderId) {
                //remove from pending array
                pendingOrderIds = ArrayMutation.removeFromArray(
                    i,
                    pendingOrderIds
                );
                order.tokenIn.safeTransfer(order.recipient, order.amountIn);

                //emit event
                emit OrderCancelled(orderId);

                //short circuit loop
                return true;
            }
        }
        return false;
    } 
library ArrayMutation {
    ///@param idx the element to remove from @param inputArray
    function removeFromArray(
        uint96 idx,
        uint96[] memory inputArray
    ) internal pure returns (uint96[] memory newList) {
        // Check that inputArray is not empty and idx is valid
        require(inputArray.length > 0, "inputArray length == 0");
        require(idx < inputArray.length, "index out of bounds");

        // Create a new array of the appropriate size
        newList = new uint96[](inputArray.length - 1);

        // Copy elements before the index
        for (uint96 i = 0; i < idx; i++) {
            newList[i] = inputArray[i];
        }

        // Copy elements after the index
        for (uint96 i = idx + 1; i < inputArray.length; i++) {
            newList[i - 1] = inputArray[i];
        }

        return newList;
    }

Impact

  • Users can be griefed, with no limitations at all! Even any PERMIT2 approvals granted for the StopLimit contract can be stolen.

PoC

Please see the references above, and follow the step-by-step attack vector as explained in the Attack Path section.

Mitigation

  1. Generate unique orderIds, by fixing the AutomationMaster::generateOrderId function to include nonces or salt values.
  2. Make sure that user's approvals to StopLimit are not getting back-runned by malicious users. Even when the 1st issue is fixed, malicious users can still create non-benefiting, losing orders for approvers.
  3. Moreover, createOrder should pass recipient, not the msg.sender to AutomationMaster's generareOrderId in reality.