Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

GMX's callback `GmxProxy::afterOrderExecution` will fail due to incorrectly checking roles

izuman

Summary

The GmxProxy contract cannot handle multiple address's for the DepositHandler, OrderHandler or WithdrawalHandler which will cause the callbacks to revert. the contract's flow cannot be completed and user's will not be minted shares after a deposit.

According to the GMX integration notes contract integrating the GMX protocol must be able to handle multiple address's for the DepositHandler, OrderHandler or WithdrawalHandler source:

Currently, the contract handle access control with a single storage variable for each of the address's and uses the following modifier:

modifier validCallback(bytes32 key, Order.Props memory order) {
require(
msg.sender == address(orderHandler) ||
msg.sender == address(liquidationHandler) ||
msg.sender == address(adlHandler),
"invalid caller"
);
require(order.addresses.account == address(this), "not mine");
_;
}
​

Vulnerability Details

GMX afterOrderExecution will incorrectly revert due to access restrictions.

Impact

Gamma protocol's contract flow will be frozen, user's will not be minted their shares, withdrawals will not be finalized and shares will not be burned.

Tools Used

Manual Review

Recommendations

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 GMX contract, e.g. RoleStore.hasRole(msg.sender, Role.CONTROLLER), this would check that the msg.sender is a valid handler.

Updates

Lead Judging Commences

n0kto Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_multiple_orderHandler_not_allowed_by_proxy

Likelihood: Low, when multiple orderHandler exist at the same time. Impact: High, some orders won’t trigger `afterOrderExecution` when they should, until the migration is complete. Leading to DoS and no share minted. Deploying a new proxy won’t change anything during all the “migration” since you cannot know which handler will respond.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.