search

sherlock-audit / 2024-05-elfi-protocol-judging

0 stars 0 forks source link

link - The `OrderFacet.cancelOrder` function run by ROLE_KEEPER may run into malicious external contracts #284

Closed sherlock-admin3 closed 1 week ago

sherlock-admin3 commented 2 weeks ago

link

High

The OrderFacet.cancelOrder function run by ROLE_KEEPER may run into malicious external contracts

Summary

The OrderFacet.createOrderRequest and OrderFacet.cancelOrder functions do not check if the marginToken (passed in by the external user) is valid or supported. This allows a callback into an arbitrary contract at the address marginToken when transferring tokens.

Vulnerability Detail

The marginToken of an Order.OrderInfo object may be any address as the OrderFacet.createOrderRequest does not apply checks on it. 

    function createOrderRequest(
        PlaceOrderParams calldata params
    ) external payable override nonReentrant {
        address account = msg.sender;
        if (
            params.posSide == Order.PositionSide.INCREASE &&
            !params.isCrossMargin
        ) {
            require(
                !params.isNativeToken || msg.value == params.orderMargin,
                "Deposit native token amount error!"
            );
            // [Suspicious-1] Do not check if token is a valid ERC20 token. 
            AssetsProcess.depositToVault(
                AssetsProcess.DepositParams(
                    account,
                    params.marginToken,
                    params.orderMargin,
                    AssetsProcess.DepositFrom.ORDER,
                    params.isNativeToken
                )
            );
        }
        Account.Props storage accountProps = Account.loadOrCreate(account);
        OrderProcess.createOrderRequest(accountProps, params, true);
    }

When the keeper fails to call executeOrder, it will call cancelOrder to cancel the order and then run into CancelOrderProcess.cancelOrder. In the case that Order.PositionSide.INCREASE == order.posSide && !order.isCrossMargin holds, it will transfer the order.orderMargin amount of order.marginToken back and finally run into the contract at order.marginToken. 

    function cancelOrder(uint256 orderId, bytes32 reasonCode) external override {
        uint256 startGas = gasleft();
        Order.OrderInfo memory order = Order.get(orderId);
        if (order.account == address(0)) {
            revert Errors.OrderNotExists(orderId);
        }
        bool isKeeper = RoleAccessControl.hasRole(RoleAccessControl.ROLE_KEEPER);
        if (!isKeeper && order.account != msg.sender) {
            revert Errors.OrderNotExists(orderId);
        }

        CancelOrderProcess.cancelOrder(orderId, order, reasonCode);
        // ......
    }
library CancelOrderProcess {
    function cancelOrder(uint256 orderId, Order.OrderInfo memory order, bytes32 reasonCode) external {
        Account.Props storage accountProps = Account.load(order.account);
        accountProps.delOrder(orderId);
        Order.remove(orderId);
        if (Order.PositionSide.INCREASE == order.posSide && order.isCrossMargin) {
            accountProps.subOrderHoldInUsd(order.orderMargin);
        } else if (Order.PositionSide.INCREASE == order.posSide && !order.isCrossMargin) {
            VaultProcess.transferOut(
                IVault(address(this)).getTradeVaultAddress(),
                order.marginToken,
                order.account,
                order.orderMargin
            );
        }
        emit CancelOrderEvent(orderId, order, reasonCode);
    }
}
library VaultProcess {
    function transferOut(address vault, address token, address receiver, uint256 amount) external returns (bool) {
        return transferOut(vault, token, receiver, amount, false);
    }
    function transferOut(
        address vault,
        address token,
        address receiver,
        uint256 amount,
        bool skipBalanceNotEnough
    ) public returns (bool) {
        if (amount == 0) {
            return false;
        }
        uint256 tokenBalance = IERC20(token).balanceOf(vault);
        if (tokenBalance >= amount) {
            Vault(vault).transferOut(token, receiver, amount);
            return true;
        } else if (!skipBalanceNotEnough) {
            revert Errors.TransferErrorWithVaultBalanceNotEnough(vault, token, receiver, amount);
        }
        return false;
    }
}
contract Vault is AccessControl {
    function transferOut(address token, address receiver, uint256 amount) external onlyRole(ADMIN_ROLE) {
        if (receiver == address(this)) {
            revert AddressSelfNotSupported(receiver);
        }
        TransferUtils.transfer(token, receiver, amount);
    }
}
library TransferUtils {
    uint256 private constant TRANSFER_GAS_LIMIT = 200000;
    error TokenTransferError(address token, address receiver, uint256 amount);
    function transfer(address token, address receiver, uint256 amount) external {
        if (amount == 0) {
            return;
        }
        bool success = transferWithGasLimit(IERC20(token), receiver, amount, TRANSFER_GAS_LIMIT);
        if (!success) {
            revert TokenTransferError(token, receiver, amount);
        }
    }
    function transferWithGasLimit(IERC20 token, address to, uint256 amount, uint256 gasLimit) internal returns (bool) {
        bytes memory data = abi.encodeWithSelector(token.transfer.selector, to, amount);
        (bool success, bytes memory returnData) = address(token).call{ gas: gasLimit }(data);
        if (!success) {
            return false;
        }
        if (returnData.length > 0 && !abi.decode(returnData, (bool))) {
            return false;
        }
        return true;
    }
}

Impact

The OrderFacet.cancelOrder function run by ROLE_KEEPER may run into malicious external contracts. At least, it can unexpectedly cost more gas than the executionFee provided by the user (cause loss of profit for the keeper).

Code Snippet

Tool used

Manual Review

Recommendation

Check if the marginToken (passed in by the external user) is valid or supported in OrderFacet.createOrderRequest.

0xELFi commented 1 week ago

_validatePlaceOrder have checked this

nevillehuang commented 6 days ago

Invalid, checked here, tokens allowed only whitelisted by config role