IllIllI - Tracking of the latest ADL block use the wrong block number on Arbitrum

[sherlock-admin]

IllIllI

medium

Tracking of the latest ADL block use the wrong block number on Arbitrum

Summary

Tracking of the latest ADL block use the wrong block number on Arbitrum

Vulnerability Detail

The call to setLatestAdlBlock() passes in block.timestamp, which on Arbitrum, is the L1 block timestamp, not the L2 timestamp on which order timestamps are based.

Impact

Tracking of whether ADL is currently required or not will be based on block numbers that are very far in the past (since Arbitrum block numbers are incremented much more quickly than Ethereum ones), so checks of whether ADL is enabled will pass, and the ADL keeper will be able to execute ADL orders whenever it wants to.

Code Snippet

Uses block.number rather than Chain.currentBlockNumber():

// File: gmx-synthetics/contracts/adl/AdlUtils.sol : AdlUtils.updateAdlState()   #1

104            MarketUtils.MarketPrices memory prices = MarketUtils.getMarketPrices(oracle, _market);
105            (bool shouldEnableAdl, int256 pnlToPoolFactor, uint256 maxPnlFactor) = MarketUtils.isPnlFactorExceeded(
106                dataStore,
107                _market,
108                prices,
109                isLong,
110                Keys.MAX_PNL_FACTOR
111            );
112    
113            setIsAdlEnabled(dataStore, market, isLong, shouldEnableAdl);
114 @>         setLatestAdlBlock(dataStore, market, isLong, block.number);
115    
116            emitAdlStateUpdated(eventEmitter, market, isLong, pnlToPoolFactor, maxPnlFactor, shouldEnableAdl);
117:       }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/adl/AdlUtils.sol#L104-L117

The block number (which is an L1 block number) is checked against the L2 oracle block numbers.

Tool used

Manual Review

Recommendation

Use Chain.currentBlockNumber() as is done everywhere else in the code base

[Jiaren-tang]

Escalate for 11 USDC. I think this should definitely be high. ADL should not be allowed to execute at any time, and if it does, it clearly the loss of fund. Because #143 is rewarded as high I think using wrong block.number allowing ADL operations to be executed when they are not supposed to be executed leads to loss of fund and deserves a high severity.

[sherlock-admin]

Escalate for 11 USDC. I think this should definitely be high. ADL should not be allowed to execute at any time, and if it does, it clearly the loss of fund. Because #143 is rewarded as high I think using wrong block.number allowing ADL operations to be executed when they are not supposed to be executed leads to loss of fund and deserves a high severity.

You've created an escalation for 11 USDC, however, the escalation amount is fixed at 10 USDC.
To update the escalation with the correct amount, please edit this comment. (do not create a new comment)

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]

I'm not sure about this one. I submitted as Medium because it's essentially a cap on profit, but as the escalator points out, slippage does create a loss. I'll leave this one to the Sherlock team to decide

[hrishibhat]

Escalation accepted

Considering this issue a high based on the slippage reasoning.

[sherlock-admin]

Escalation accepted

Considering this issue a high based on the slippage reasoning.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.