simon135 - Receiver can be an malicious and gain free profit and not get liquidated/adling

[sherlock-admin]

simon135

high

Receiver can be an malicious and gain free profit and not get liquidated/adling

Summary

If the receiver is smart contract and protocol is going to send eth on the .call we specify large gas revert by having a big bytes memory data and the keeper will not make the tx and when the price is right they can change it and make the call execute getting free profit

Vulnerability Detail

even though the bytes memory data is commented out the high-level solidity call still receives the bytes so an attacker can craft a huge string of data that gas will be too much for the keepers to process and the tx will sit until tx won't revert.

Impact

An attacker can cause this to gain free profits like when they have a long position on eth and the price of eth is falling they can make the tx revert but when the price of eth is high they can make the tx go through and they won't even get liquidated because the keeper cant liquidate them or adl them. This attack only works on shouldUnwrapNativeToken=true if the protocol cant liquidate or position that is really bad and that can cause bad debt and if they cant adl that will also cause bad debt and profit for the attacker

Code Snippet

when decreasing order/ or liquidating the position or adling it will revert and not work

        if (order.swapPath().length == 0) {
            MarketToken(payable(order.market())).transferOut(
                result.outputToken,
                order.receiver(),
                result.outputAmount,
                order.shouldUnwrapNativeToken()

we can do the same thing for swapping and wait until eth is at good price

        SwapUtils.swap(SwapUtils.SwapParams(
            params.contracts.dataStore,
            params.contracts.eventEmitter,
            params.contracts.oracle,
            params.contracts.orderVault,
            params.order.initialCollateralToken(),
            params.order.initialCollateralDeltaAmount(),
            params.swapPathMarkets,
            params.order.minOutputAmount(),
            params.order.receiver(),
            params.order.shouldUnwrapNativeToken()
        ));

https://github.com/gmx-io/gmx-synthetics/blob/c4814a6c4c9269b9367fb6d462e30ff6f37480e5/contracts/order/DecreaseOrderUtils.sol#L169-L175

Tool used

Manual Review

Recommendation

make low-level call in yul

[snn20]

Escalate for 10 USDC I have seen this type of vuln on c4 but the attack here is kind of like #139 but a different part of the code but with a mass return data in memory coming from the receiver to halt something or not get liquidated or adl. https://github.com/ethereum/solidity/issues/12306

      (bool success, /* bytes memory data */) = payable(receiver).call{ value: amount, gas: gasLimit }("");

https://github.com/gmx-io/gmx-synthetics/blob/c4814a6c4c9269b9367fb6d462e30ff6f37480e5/contracts/token/TokenUtils.sol#L133

[sherlock-admin]

Escalate for 10 USDC I have seen this type of vuln on c4 but the attack here is kind of like #139 but a different part of the code but with a mass return data in memory coming from the receiver to halt something or not get liquidated or adl. https://github.com/ethereum/solidity/issues/12306

      (bool success, /* bytes memory data */) = payable(receiver).call{ value: amount, gas: gasLimit }("");

https://github.com/gmx-io/gmx-synthetics/blob/c4814a6c4c9269b9367fb6d462e30ff6f37480e5/contracts/token/TokenUtils.sol#L133

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[IllIllI000]

The code that the escalation comment points to uses a gas: argument to the call() function, which means the full string passed back through normal means is limited to taking that much gas. As long as the string isn't read back, there's no issue, and the submission incorrectly believes that a large string will cause a revert, which it won't since the code checks the return value of the call(). When it fails, the very next lines send WNT instead, and there is not advantage gained over the system

[hrishibhat]

Escalation rejected

As pointed out by the Lead watson's comment, when the return value of the call is false the wnt is sent instead. Do not see any valid issue here

[sherlock-admin]

Escalation rejected

As pointed out by the Lead watson's comment, when the return value of the call is false the wnt is sent instead. Do not see any valid issue here

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.