055.md

Decent Smoke Owl

High

Dangerous overflow lead to loss of funds

Summary

Explicitly casting a uint256 to uint160 can cause an overflow. This won't revert the transaction but can break protocol calculations.

Root Cause

Issue is presented in all places where functions have uint256 parameter which is later casted to uint160 because of permit functionality. I am going to show one example. In modifyOrder() in Bracket contract we have the following logic to increase the order amountIn:

function modifyOrder(
        uint96 orderId,
        uint256 _takeProfit,
        uint256 _stopPrice,
        uint256 amountInDelta,
        IERC20 _tokenOut,
        address _recipient,
        uint16 _takeProfitSlippage,
        uint16 _stopSlippage,
        bool permit,
        bool increasePosition,
        bytes calldata permitPayload
)

        if (amountInDelta != 0) {
            if (increasePosition) {
                newAmountIn += amountInDelta;
                //take funds via permit2
                if (permit) {
                    handlePermit(
                        order.recipient,
                        permitPayload,
                        uint160(amountInDelta), //@audit dangerous overflow
                        address(order.tokenIn)
                    );
                } else {
                    //legacy transfer, assume prior approval
                    order.tokenIn.safeTransferFrom(
                        order.recipient,
                        address(this),
                        amountInDelta
                    );
                }
            }

		Order memory newOrder = Order({
            orderId: orderId,
            takeProfit: _takeProfit,
            stopPrice: _stopPrice,
@>          amountIn: newAmountIn,
            tokenIn: order.tokenIn,
            tokenOut: _tokenOut,
            feeBips: order.feeBips,
            takeProfitSlippage: _takeProfitSlippage,
            stopSlippage: _stopSlippage,
            recipient: _recipient,
            direction: MASTER.getExchangeRate(order.tokenIn, _tokenOut) >
                _takeProfit
        });

if amountInDelta is type(uint160).max + X, uint160(amountInDelta) would be equal to X - 1 thus taking only X - 1 from the user in case of permit scenario and updating the order with the full provided amountInDelta.

modifyOrder(): https://github.com/sherlock-audit/2024-11-oku/blob/ee3f781a73d65e33fb452c9a44eb1337c5cfdbd6/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L216C5-L297C6

Internal pre-conditions

N/A

External pre-conditions

N/A

Attack Path

Users modifying orders with amountInDelta > type(uint160).max

Impact

Incorrect accounting for orders and funds provided by users. It is expected that the amountIn of orders is provided by users.

PoC

You can verify the behavior with the following contract

// SPDX-License-Identifier: MIT

pragma solidity 0.8.20;

contract Test {
    function test(uint delta) external pure returns(uint160){
        return uint160(uint256(type(uint160).max + delta));
    }
}

Mitigation

In all places where permit is going to be used, make the user provided parameter to be uint160.