sherlock-audit / 2023-02-gmx-judging

17 stars 11 forks source link

IllIllI - Malicious order keepers can trigger the cancellation of any order, with old blocks #163

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

IllIllI

medium

Malicious order keepers can trigger the cancellation of any order, with old blocks

Summary

Malicious order keepers can trigger the cancellation of any order by providing oracle prices from blocks unrelated to the order

Vulnerability Detail

Each order has its own requirements about what block ranges it expects. The error handler for each order only allows orders to be retried if the keeper provided empty/invalid prices. The error handlers make no such allowances for invalid blocks.

The order keepers are supposed to be a decentralized network, so it's likely that at some point there will be a bad actor, or one who uses the rules to their advantage, at the expense of other users.

Impact

Orders will be canceled rather than retried by keepers that would have executed the order properly. This may lead to position liquidations.

Code Snippet

These are three revert errors that are not caught by the various error handlers:

    function revertOracleBlockNumbersAreNotEqual(uint256[] memory oracleBlockNumbers, uint256 expectedBlockNumber) internal pure {
        revert OracleBlockNumbersAreNotEqual(oracleBlockNumbers, expectedBlockNumber);
    }

    function revertOracleBlockNumbersAreSmallerThanRequired(uint256[] memory oracleBlockNumbers, uint256 expectedBlockNumber) internal pure {
        revert OracleBlockNumbersAreSmallerThanRequired(oracleBlockNumbers, expectedBlockNumber);
    }

    function revertOracleBlockNumberNotWithinRange(
        uint256[] memory minOracleBlockNumbers,
        uint256[] memory maxOracleBlockNumbers,
        uint256 blockNumber
    ) internal pure {
        revert OracleBlockNumberNotWithinRange(minOracleBlockNumbers, maxOracleBlockNumbers, blockNumber);
    }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/oracle/OracleUtils.sol#L276-L290

Tool used

Manual Review

Recommendation

Add checks for these errors, and make the functions result in a revert rather than a cancellation

xvi10 commented 1 year ago

Fix in https://github.com/gmx-io/gmx-synthetics/commit/4a5981a1f006468cd78dd974110d0a8be23b5fc9#diff-d1cd88df390b68e193ecd449d918fbfccb8c24f39cbb3f4c32a659e932122d8fR291