Liquidity Management

Description:

Take a look at the following statement from GMX github's Integration Notes section:

When creating a callback contract, the callback contract may need to whitelist the DepositHandler, OrderHandler or WithdrawalHandler, it should be noted that new versions of these handlers may be deployed as new code is added to the handlers, it is also possible for two handlers to temporarily exist at the same time, e.g. OrderHandler(1), OrderHandler(2), due to this, the callback contract should be able to whitelist and simultaneously accept callbacks from multiple DepositHandlers, OrderHandlers and WithdrawalHandlers

Source: https://github.com/gmx-io/gmx-synthetics/blob/main/README.md#integration-notes

In the above statement we can see the warning from GMX team, which states that the callback contract should be able to whitelist and simultaneously accept callbacks from multiple DepositHandlers, OrderHandlers and WithdrawalHandlers.

simultaneously in this case means that the callback contract can be called from any of the existing 2 handlers of the same type, which is a problem for Gamma protocol, because in the current implementation, they can only accept calls from 1 handler of each type:

In the code snippet provided above we can see that this modifier checks only 1 of each type of handler, which does not follow integration advice from GMX.

Owner can also set just 1 handler of each type:

Impact:

If GmxProxy::afterOrderExecution or GmxProxy::afterOrderCancellation function gets called by a GMX handler which is not whitelisted, then the call will revert (GMX skips over it and completes order execution), which results in PerpetualVault's state not being updated.

Likelihood:

This issue is fairly likely to happen, because GMX undergoes upgrades frequently and during the timeline when these upgrades happen they can switch the handler that is used by their keepers at any time. When they will switch is hard to tell, because when they decide on a date, they can unexpectedly change it, due to internal GMX decisions, also it's possible that the handler used by the GMX keepers can be switched back and forth due to unexpected issues, so it is not likely that the owner of GmxProxy can keep setting the correct handler address always at the right time.

Recommended Mitigation:

Follow this GMX's advice, which can be found in the link provided above:

For callback contracts instead of maintaining a separate whitelist for DepositHandlers, OrderHandlers, WithdrawalHandlers, a possible solution would be to validate the role of the msg.sender in the RoleStore, e.g. RoleStore.hasRole(msg.sender, Role.CONTROLLER), this would check that the msg.sender is a valid handler