Init Capital Invitational - said's results

Lines of code

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L504-L526

Vulnerability details

Impact

Due to improper validation, anyone who has positions inside the MarginTradingHook can update orders created by other users. This vulnerability can be exploited to modify order information, such as collateral amount, trigger/limit price, then execute fillOrder, causing losses for the original creator

Proof of Concept

It can be observed that as long as users have position (initPosId != 0) and the order is active, they can provide any valid _orderId an update the order value.

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L504-L526

    function updateOrder(
        uint _posId,
        uint _orderId,
        uint _triggerPrice_e36,
        address _tokenOut,
        uint _limitPrice_e36,
        uint _collAmt
    ) external {
        _require(_collAmt != 0, Errors.ZERO_VALUE);
        Order storage order = __orders[_orderId];
        _require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
        uint initPosId = initPosIds[msg.sender][_posId];
        _require(initPosId != 0, Errors.POSITION_NOT_FOUND);
        MarginPos memory marginPos = __marginPositions[initPosId];
        uint collAmt = IPosManager(POS_MANAGER).getCollAmt(initPosId, marginPos.collPool);
        _require(_collAmt <= collAmt, Errors.INPUT_TOO_HIGH);

        order.triggerPrice_e36 = _triggerPrice_e36;
        order.limitPrice_e36 = _limitPrice_e36;
        order.collAmt = _collAmt;
        order.tokenOut = _tokenOut;
        emit UpdateOrder(initPosId, _orderId, _tokenOut, _triggerPrice_e36, _limitPrice_e36, _collAmt);
    }

As mentioned before, this can be maliciously updated with value that will cause loss for the order creator, and execute the order to steal token from the order.

Tools Used

manual review

Recommended Mitigation Steps

Verify only order creator can update the order.

    function updateOrder(
        uint _posId,
        uint _orderId,
        uint _triggerPrice_e36,
        address _tokenOut,
        uint _limitPrice_e36,
        uint _collAmt
    ) external {
        _require(_collAmt != 0, Errors.ZERO_VALUE);
        Order storage order = __orders[_orderId];
        _require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
        uint initPosId = initPosIds[msg.sender][_posId];
        _require(initPosId != 0, Errors.POSITION_NOT_FOUND);
+       _require(order.initPosId == initPosId, Errors.INVALID_INPUT);
        MarginPos memory marginPos = __marginPositions[initPosId];
        uint collAmt = IPosManager(POS_MANAGER).getCollAmt(initPosId, marginPos.collPool);
        _require(_collAmt <= collAmt, Errors.INPUT_TOO_HIGH);

        order.triggerPrice_e36 = _triggerPrice_e36;
        order.limitPrice_e36 = _limitPrice_e36;
        order.collAmt = _collAmt;
        order.tokenOut = _tokenOut;
        emit UpdateOrder(initPosId, _orderId, _tokenOut, _triggerPrice_e36, _limitPrice_e36, _collAmt);
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L504-L526

Vulnerability details

Impact

When an order is created, it checks whether the order.tokenOut is equal to marginPos.baseAsset or _tokenOut is equal to marginPos.quoteAsset. However, when tokenOut is updated via updateOrder, it can be changed to an arbitrary token. This has the potential to be exploited by changing tokenOut to a high-value token right before the executor executes the fillOrder.

Proof of Concept

It can be observed that order's creator can update order.tokenOut to arbitrary token.

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L504-L526

    function updateOrder(
        uint _posId,
        uint _orderId,
        uint _triggerPrice_e36,
        address _tokenOut,
        uint _limitPrice_e36,
        uint _collAmt
    ) external {
        _require(_collAmt != 0, Errors.ZERO_VALUE);
        Order storage order = __orders[_orderId];
        _require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
        uint initPosId = initPosIds[msg.sender][_posId];
        _require(initPosId != 0, Errors.POSITION_NOT_FOUND);
        MarginPos memory marginPos = __marginPositions[initPosId];
        uint collAmt = IPosManager(POS_MANAGER).getCollAmt(initPosId, marginPos.collPool);
        _require(_collAmt <= collAmt, Errors.INPUT_TOO_HIGH);

        order.triggerPrice_e36 = _triggerPrice_e36;
        order.limitPrice_e36 = _limitPrice_e36;
        order.collAmt = _collAmt;
        order.tokenOut = _tokenOut;
        emit UpdateOrder(initPosId, _orderId, _tokenOut, _triggerPrice_e36, _limitPrice_e36, _collAmt);
    }

This can be exploited by the order's creator. When the executor is about to execute the fillOrder, the order creator can front-run the operation by changing the order.tokenOut to a token with a high value. Consequently, the executor will transfer the amount that needs to be transferred but using this new high-value token. But this require hook already have approval from the executor to manage the new high value token.

Tools Used

Manual review

Recommended Mitigation Steps

Validate the new token when updateOrder is called :

    function updateOrder(
        uint _posId,
        uint _orderId,
        uint _triggerPrice_e36,
        address _tokenOut,
        uint _limitPrice_e36,
        uint _collAmt
    ) external {
        _require(_collAmt != 0, Errors.ZERO_VALUE);
        Order storage order = __orders[_orderId];
        _require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
        uint initPosId = initPosIds[msg.sender][_posId];
        _require(initPosId != 0, Errors.POSITION_NOT_FOUND);
        MarginPos memory marginPos = __marginPositions[initPosId];
        uint collAmt = IPosManager(POS_MANAGER).getCollAmt(initPosId, marginPos.collPool);
        _require(_collAmt <= collAmt, Errors.INPUT_TOO_HIGH);
+       _require(_tokenOut == marginPos.baseAsset || _tokenOut == marginPos.quoteAsset, Errors.INVALID_INPUT);

        order.triggerPrice_e36 = _triggerPrice_e36;
        order.limitPrice_e36 = _limitPrice_e36;
        order.collAmt = _collAmt;
        order.tokenOut = _tokenOut;
        emit UpdateOrder(initPosId, _orderId, _tokenOut, _triggerPrice_e36, _limitPrice_e36, _collAmt);
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L539-L563 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L387

Vulnerability details

Impact

limitPrice_e36 acts as slippage protection for the order creator, depending on the trade/order position (whether long or short). A higher or lower limit price impacts the tokenOut amount that needs to be transferred to the order's creator. However, when the executor executes fillOrder, it can be front-run by the order creator to update limitPrice_e36 and steal tokens from the executor.

Proof of Concept

When fillOrder is executed, it will calculate the amtOut that needs to be transferred to order.recipient by calling _calculateFillOrderInfo.

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L532-L564

    function _calculateFillOrderInfo(Order memory _order, MarginPos memory _marginPos, address _collToken)
        internal
        returns (uint amtOut, uint repayShares, uint repayAmt)
    {
        (repayShares, repayAmt) = _calculateRepaySize(_order, _marginPos);
        uint collTokenAmt = ILendingPool(_marginPos.collPool).toAmtCurrent(_order.collAmt);
        // NOTE: all roundings favor the order owner (amtOut)
        if (_collToken == _order.tokenOut) {
            if (_marginPos.isLongBaseAsset) {
                // long eth hold eth
                // (2 * 1500 - 1500) = 1500 / 1500 = 1 eth
                // ((c * limit - borrow) / limit
>>>             amtOut = collTokenAmt - repayAmt * ONE_E36 / _order.limitPrice_e36;
            } else {
                // short eth hold usdc
                // 2000 - 1 * 1500 = 500 usdc
                // (c - borrow * limit)
>>>             amtOut = collTokenAmt - (repayAmt * _order.limitPrice_e36 / ONE_E36);
            }
        } else {
            if (_marginPos.isLongBaseAsset) {
                // long eth hold usdc
                // (2 * 1500 - 1500) = 1500 usdc
                // ((c * limit - borrow)
>>>             amtOut = (collTokenAmt * _order.limitPrice_e36).ceilDiv(ONE_E36) - repayAmt;
            } else {
                // short eth hold eth
                // (3000 - 1 * 1500) / 1500 = 1 eth
                // (c - borrow * limit) / limit
>>>             amtOut = (collTokenAmt * ONE_E36).ceilDiv(_order.limitPrice_e36) - repayAmt;
            }
        }
    }

As can be observed, it heavily relies on limitPrice_e36 to calculate amtOut. A malicious order creator can front-run the execution of fillOrder and steal assets from the executor by changing limitPrice_e36, resulting in a high value of amtOut depending on the order's position.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider to add limitPrice_e36 check inside the fillOrder if it bigger than min/max provided limit price, revert the operation.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L373 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L380

Vulnerability details

Impact

When fillOrder is triggered, and the order's collateral size inside the position manager is empty, it should change the order status to canceled. However, due to mistakenly updating memory data instead of the storage __orders data, it will not be canceled properly.

Proof of Concept

It can be observed that when collateral size is empty, fillOrder will try to update order memory data status to OrderStatus.Cancelled and return the call.

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L379-L383

    function fillOrder(uint _orderId) external {
>>>     Order memory order = __orders[_orderId];
        _require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
        MarginPos memory marginPos = __marginPositions[order.initPosId];
        address collToken = ILendingPool(marginPos.collPool).underlyingToken();
        address borrToken = ILendingPool(marginPos.borrPool).underlyingToken();
        // if position is empty, cancel order
        if (IPosManager(POS_MANAGER).getCollAmt(order.initPosId, marginPos.collPool) == 0) {
>>>         order.status = OrderStatus.Cancelled;
            emit CancelOrder(order.initPosId, _orderId);
            return;
        }
        // validate trigger price condition
        _validateTriggerPrice(order, marginPos);
        // calculate fill order info
        (uint amtOut, uint repayShares, uint repayAmt) = _calculateFillOrderInfo(order, marginPos, collToken);
        // transfer in repay tokens
        IERC20(borrToken).safeTransferFrom(msg.sender, address(this), repayAmt);
        // transfer order owner's desired tokens to the specified recipient
        IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);
        // repay tokens
        _ensureApprove(borrToken, repayAmt);
        IInitCore(CORE).repay(marginPos.borrPool, repayShares, order.initPosId);
        // decollateralize coll pool tokens to executor
        IInitCore(CORE).decollateralize(order.initPosId, marginPos.collPool, order.collAmt, msg.sender);
        // update order status
        __orders[_orderId].status = OrderStatus.Filled;
        emit FillOrder(order.initPosId, _orderId, order.tokenOut, amtOut);
    }

However, this will not change the storage value of the order inside the __orders data. This will mislead users into assuming that the order is canceled when, in fact, it is not.

Tools Used

Manual review.

Recommended Mitigation Steps

Update __orders[_orderId].status storage data instead :

    function fillOrder(uint _orderId) external {
        Order memory order = __orders[_orderId];
        _require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
        MarginPos memory marginPos = __marginPositions[order.initPosId];
        address collToken = ILendingPool(marginPos.collPool).underlyingToken();
        address borrToken = ILendingPool(marginPos.borrPool).underlyingToken();
        // if position is empty, cancel order
        if (IPosManager(POS_MANAGER).getCollAmt(order.initPosId, marginPos.collPool) == 0) {
-            order.status = OrderStatus.Cancelled;
+            __orders[_orderId].status = OrderStatus.Cancelled;
            emit CancelOrder(order.initPosId, _orderId);
            return;
        }
        // validate trigger price condition
        _validateTriggerPrice(order, marginPos);
        // calculate fill order info
        (uint amtOut, uint repayShares, uint repayAmt) = _calculateFillOrderInfo(order, marginPos, collToken);
        // transfer in repay tokens
        IERC20(borrToken).safeTransferFrom(msg.sender, address(this), repayAmt);
        // transfer order owner's desired tokens to the specified recipient
        IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);
        // repay tokens
        _ensureApprove(borrToken, repayAmt);
        IInitCore(CORE).repay(marginPos.borrPool, repayShares, order.initPosId);
        // decollateralize coll pool tokens to executor
        IInitCore(CORE).decollateralize(order.initPosId, marginPos.collPool, order.collAmt, msg.sender);
        // update order status
        __orders[_orderId].status = OrderStatus.Filled;
        emit FillOrder(order.initPosId, _orderId, order.tokenOut, amtOut);
    }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L422

Vulnerability details

Impact

When users reduce their positions and provide collateral tokens as _param.tokenOut, it will sets the swap type to SwapType.CloseExactOut. It then checks whether the balance of the token out is equal to the provided swapInfo.amtOut or the repay amount. However, this check is too strict and can be easily DoSed by transferring a dust amount of tokens to the hook.

Proof of Concept

It ca be observed that when _param.tokenOut is collToken , it will set swapType with SwapType.CloseExactOut. This will swap tokens collToken to borrToken with only token amount needed for repaying the debt.

    function _reducePosInternal(ReducePosInternalParam memory _param) internal returns (uint amtOut, uint health_e18) {
        MarginPos memory marginPos = __marginPositions[_param.initPosId];
        // check collAmt & repay shares
        _require(
            _param.collAmt <= IPosManager(POS_MANAGER).getCollAmt(_param.initPosId, marginPos.collPool),
            Errors.INPUT_TOO_HIGH
        );
        _require(
            _param.repayShares <= IPosManager(POS_MANAGER).getPosDebtShares(_param.initPosId, marginPos.borrPool),
            Errors.INPUT_TOO_HIGH
        );

        address collToken = ILendingPool(marginPos.collPool).underlyingToken();
        address borrToken = ILendingPool(marginPos.borrPool).underlyingToken();
        _require(_param.tokenOut == collToken || _param.tokenOut == borrToken, Errors.INVALID_INPUT);

        uint repayAmt = ILendingPool(marginPos.borrPool).debtShareToAmtCurrent(_param.repayShares);
        _ensureApprove(borrToken, repayAmt);

        // 1. decollateralize collateral tokens
        // 2. redeem underlying collateral tokens
        // 3. callback (perform swap from coll -> borr)
        // 4. repay borrow tokens
        bytes[] memory multicallData = new bytes[](4);
        multicallData[0] = abi.encodeWithSelector(
            IInitCore(CORE).decollateralize.selector,
            _param.initPosId,
            marginPos.collPool,
            _param.collAmt,
            marginPos.collPool
        );
        multicallData[1] = abi.encodeWithSelector(IInitCore(CORE).burnTo.selector, marginPos.collPool, address(this));
        {
            // if expect token out = borr token -> swap all (exact-in)
            // if expect token out = coll token -> swap enough to repay (exact-out)
>>>         SwapType swapType = _param.tokenOut == borrToken ? SwapType.CloseExactIn : SwapType.CloseExactOut;
            SwapInfo memory swapInfo = SwapInfo(_param.initPosId, swapType, collToken, borrToken, repayAmt, _param.data);
            multicallData[2] =
                abi.encodeWithSelector(IInitCore(CORE).callback.selector, address(this), 0, abi.encode(swapInfo));
        }
        multicallData[3] = abi.encodeWithSelector(
            IInitCore(CORE).repay.selector, marginPos.borrPool, _param.repayShares, _param.initPosId
        );

        // do multicall
        IMulticall(CORE).multicall(multicallData);
        amtOut = IERC20(_param.tokenOut).balanceOf(address(this));
        // slippage control check
        _require(amtOut >= _param.minAmtOut, Errors.SLIPPAGE_CONTROL);
        // transfer tokens out
        _transmitTokenOut(_param.tokenOut, amtOut, _param.returnNative);

        // check slippage control on position's health
        health_e18 = _validateHealth(_param.initPosId, _param.minHealth_e18);
        emit ReducePos(_param.initPosId, _param.tokenOut, amtOut, _param.collAmt, repayAmt);
    }

However, when swap is performed inside coreCallback, it will strictly check tokenOut (borrToken) balance inside the contract equal with swapInfo.amtOut (repayAmt).

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/main/contracts/hook/MarginTradingHook.sol#L422

    function coreCallback(address _sender, bytes calldata _data) external payable returns (bytes memory result) {
        _require(msg.sender == CORE, Errors.NOT_INIT_CORE);
        _require(_sender == address(this), Errors.NOT_AUTHORIZED);
        SwapInfo memory swapInfo = abi.decode(_data, (SwapInfo));
        MarginPos memory marginPos = __marginPositions[swapInfo.initPosId];
        uint amtIn = IERC20(swapInfo.tokenIn).balanceOf(address(this));
        IERC20(swapInfo.tokenIn).safeTransfer(swapHelper, amtIn); // transfer all token in to swap helper
        // swap helper swap token
        IBaseSwapHelper(swapHelper).swap(swapInfo);
        uint amtOut = IERC20(swapInfo.tokenOut).balanceOf(address(this));
        if (swapInfo.swapType == SwapType.OpenExactIn) {
            // transfer to coll pool to mint
            IERC20(swapInfo.tokenOut).safeTransfer(marginPos.collPool, amtOut);
            emit SwapToIncreasePos(swapInfo.initPosId, swapInfo.tokenIn, swapInfo.tokenOut, amtIn, amtOut);
        } else {
            // transfer to borr pool to repay
            uint amtSwapped = amtIn;
            if (swapInfo.swapType == SwapType.CloseExactOut) {
                // slippage control to make sure that swap helper swap correctly
  >>>           _require(IERC20(swapInfo.tokenOut).balanceOf(address(this)) == swapInfo.amtOut, Errors.SLIPPAGE_CONTROL);
                amtSwapped -= IERC20(swapInfo.tokenIn).balanceOf(address(this));
            }
            emit SwapToReducePos(swapInfo.initPosId, swapInfo.tokenIn, swapInfo.tokenOut, amtSwapped, amtOut);
        }
        result = abi.encode(amtOut);
    }

This check is too strict and vulnerable to DoS attack. Attacker can send a dust amount of tokenOut to the hook before the operation. As a result, reducePos will fail when a user wants collateral tokens as _param.tokenOut, preventing the users to close/reduce position.

Tools Used

Manual review.

Recommended Mitigation Steps

Change the check to the following :

//...
-            _require(IERC20(swapInfo.tokenOut).balanceOf(address(this)) == swapInfo.amtOut, Errors.SLIPPAGE_CONTROL);
+            _require(IERC20(swapInfo.tokenOut).balanceOf(address(this)) >= swapInfo.amtOut, Errors.SLIPPAGE_CONTROL);
//...

Assessed type

DoS