IllIllI - Oracles are vulnerable to cross-chain replay attacks

[sherlock-admin]

IllIllI

medium

Oracles are vulnerable to cross-chain replay attacks

Summary

Oracles are vulnerable to cross-chain replay attacks

Vulnerability Detail

If there's a hard fork (e.g. miner vs proof of stake), signatures/txns submitted on one chain can be replayed on the other chain.

Impact

Keepers on the forked chain can use oracle prices from the original chain, even though they may not be correct for the current chain (e.g. USDC's price on the fork may be worth zero until Circle adds support on that fork), leading to invalid execution prices and or liquidations.

Code Snippet

The SALT used in signatures is hard-coded to an immutable variable in the constructor, and uses the block.chainid from the time of construction:

// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle.constructor()   #1

108        constructor(
109            RoleStore _roleStore,
110            OracleStore _oracleStore
111        ) RoleModule(_roleStore) {
112            oracleStore = _oracleStore;
113    
114            // sign prices with only the chainid and oracle name so that there is
115            // less config required in the oracle nodes
116 @>         SALT = keccak256(abi.encode(block.chainid, "xget-oracle-v1"));
117:       }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/oracle/Oracle.sol#L108-L117

Signatures use the SALT without also including the block.chainid separately

Tool used

Manual Review

Recommendation

Include the block.chainid in the signature, separately from the SALT, and ensure that the value is looked up each time

[hack3r-0m]

Escalate for 10 USDC

This finding should be invalid, because keepers are trusted entity as mentioned in REAMDE and signature of keeper on longest chain cannot be used on forks because of check that only keeper can update price.

[sherlock-admin]

Escalate for 10 USDC

This finding should be invalid, because keepers are trusted entity as mentioned in REAMDE and signature of keeper on longest chain cannot be used on forks because of check that only keeper can update price.

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 readme says, Order keepers and frozen order keepers could potentially extract value through transaction ordering, delayed transaction execution etc, this will be partially mitigated with a keeper network so they are not fully-trusted entities

[hrishibhat]

Escalation rejected

Admin is restricted and keepers are trusted only to perform the actions assigned to them. As pointed out by the Lead watson, Keepers are not fully trusted entities

[sherlock-admin]

Escalation rejected

Admin is restricted and keepers are trusted only to perform the actions assigned to them. As pointed out by the Lead watson, Keepers are not fully trusted entities

This issue's escalations have been rejected!

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

[xvi10]

Fix in gmx-io/gmx-synthetics#113