Chainlink Automation

The Chainlink automation keepers are configured to send 5,000,000 gas with each execution, however GMX deposits, withdrawals, and orders may often require more than 5,000,000 gas to execute.

In the general.ts configuration file the increaseOrderGasLimit and decreaseOrderGasLimit are configured to 4_000_000, additionally the minAdditionalGasForExecution is configured to 1_000_000. Therefore any increase or decrease order including a callback with a nonzero callbackGasLimit cannot be executed by the Chainlink automation keepers.

Additionally the singleSwapGasLimit is configured to 1_000_000, therefore deposits and withdrawals with 4 or more total swaps in the longTokenSwapPath and shortTokenSwapPath are unable to be executed by the Chainlink automation keepers.

Increase the configured gas amount to as high as 15,000,000 to ensure that even the most expensive of actions can be executed on GMX V2.

Chainlink Team: The configuration was adjusted to 13,000,000 gas after discussing with the GMX team.

The longTokenSwapPath and the shortTokenSwapPath of a withdrawal cannot be decoded. This is because WithdrawalEventUtils.emitWithdrawalCreated does not emit the swap paths on a Withdrawal.

Consequently, the withdrawal automation becomes unusable whenever a user requires a swap post-withdrawal.

Add the longTokenSwapPath and shortTokenSwapPath to the WithdrawalCreated event. Afterwards, modify WithdrawalAutomation to decode these paths and _addPropsToMapping to set the necessary feeds.

GMX Team: The necessary event data was added in commit 3122a9.

Chainlink Team: The longTokenSwapPath and shortTokenSwapPath logic was added to the WithdrawalAutomation in commit fc35f03.

There is no access control for the performUpkeep function, therefore any arbitrary address may execute an order. On a network with a public mempool, such as Avalanche, any arbitrary user may observe the Chainlink keeper’s transaction and copy the performData to execute their own deposit, withdrawal, or order.

A malicious actor can therefore front-run the execution of other user’s orders to manipulate price impact such that the actor stands to gain a profit at the user’s detriment.

Add access controls to the performUpkeep function in the MarketAutomation, DepositAutomation, and WithdrawalAutomation contracts.

Once access controls are added to the performUpkeep function, it should be noted that the permissioned caller still holds the ability to decide execution ordering, controlling the price impact experienced by orders.

Chainlink Team: The suggested onlyForwarder access control was added in commit 0a7edc2.

Using the performUpkeep function, the Chainlink keeper may execute any order, even if the order is not a MarketIncrease, MarketDecrease, or MarketSwap.

However the MarketAutomation contract is explicitly designed to execute only market orders, therefore the scope of which orders can be executed with the performUpkeep function should be limited by validating the orderType from the dataStore.

Validate that the order being executed is indeed a MarketIncrease, MarketDecrease, or MarketSwap.

Chainlink Team: Acknowledged.

Deposits, withdrawals and orders may use swap paths including any token supported on the GMX V2 platform.

Therefore every supported token must be assigned a valid functioning feedId, otherwise deposits, withdrawals and orders may be unable to be executed using the Chainlink keeper or may become cancelled unexpectedly.

Ensure that every token on the GMX V2 platform is supported by a valid feedId.

Additionally, implement validation in the checkLog function such that if a feedId is not configured for a token that is necessary for the action, the action is not executed by the Chainlink keeper and is instead executed by the default keeper.

Chainlink Team: The recommended validation was added in commit a21b0b9.

The comment on line 51 reads a feed lookup lookup where the word lookup is repeated twice.

Remove the second instance of lookup.

Chainlink Team: The recommendation was implemented in commit ffed912.

The DepositAutomation, MarketAutomation, and WithdrawalAutomation contracts have no receive function and therefore cannot receive gas remuneration from GMX V2 in native tokens.

GMX V2 handles this by re-wrapping the native tokens and sending them to the keeper. However the keeper must pay for this additional re-wrapping gas expenditure on every transaction.

To avoid this additional gas expenditure on every transaction, implement a receive function, with a way to retrieve the accumulated native tokens.

Chainlink Team: Acknowledged.

The ILogAutomation contract documentation mentions that a cannotExecute modifier should be added to the checkLog function to ensure it can never be errantly called in a transaction, only simulated.

However the checkLog functions in the DepositAutomation, MarketAutomation, and WithdrawalAutomation contracts lack any cannotExecute or similar modifier.

Consider whether a cannotExecute modifier should be added.

If so, implement a cannotExecute modifier in the GMXAutomationBase contract and add it to the checkLog functions in the DepositAutomation, MarketAutomation, and WithdrawalAutomation contracts.

Chainlink Team: Acknowledged.

In the DepositAutomation and WithdrawalAutomation contracts the comment on line 53 states // Decode Event Log 1, however both "DepositCreated" and "WithdrawalCreated" are emitted with Event Log 2.

Update the comment to // Decode Event Log 2.

Chainlink Team: The recommendation was implemented in commit ffed912.

In the DepositAutomation, WithdrawalAutomation, and MarketAutomation contracts the i_depositHandler, i_withdrawalHandler, and i_orderHandler variables are declared as immutable. Additionally, in the GMXAutomationBase contract the i_dataStore and i_reader variables are declared immutable.

However it is possible that the respective contracts are upgraded or replaced, meaning that the existing Chainlink Automation contract is no longer functional. Therefore a new Automation contract would need to be deployed and whitelisted as a valid keeper in the event of a handler contract upgrade.

Consider if the owner address should be able to update these contract addresses in the event of an upgrade.

Chainlink Team: Redeployment of an Automation contract is preferred on the Chainlink operational side.

The StreamsLookup error labels the second to last parameter as time, it is unclear whether this is intended to be a timestamp.

However a block number is provided for the time in the StreamsLookup error in the checkLog function in each of the DepositAutomation, WithdrawalAutomation, and MarketAutomation contracts.

Verify whether the time parameter is intended to be a block number or timestamp, consider documenting what the time parameter represents in the StreamsLookupCompatibleInterface interface.

Chainlink Team: This is expected.

In the NatSpec for the _flushMapping function, the addresses returned value is omitted.

Add the addresses value to the NatSpec for the _flushMapping function.

Chainlink Team: The recommendation was implemented in commit ffed912.

In the _toHexString function, the comment on line 101 states // Fixed buffer size for hexadecimal convertion, however conversion is misspelled as convertion.

Replace convertion with conversion.

Chainlink Team: The recommendation was implemented in commit ffed912.