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

Incorrect validation of callback caller

vinica_boy

Summary

GmxProxy correctly implements a validCallback modifier where msg.sender and order.addresses.account are verified. Based on GMX V2 implementation, it is possible to have more than one OrderHandler which is incompatible with the current implementation.

  • 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:

Vulnerability Details

Currently, only single orderHandler can call the callback functions of GmxProxy:

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");
_;
}

Impact

In case of update in GMX, it is possible that callback calls from GMX will fail as Gamma cannot support multiple orderHandler. This will lead to complete brick of the protocol and innability to process all actions related to it.

Tools Used

Manual review.

Recommendations

From the same README:

  • 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

Updates

Lead Judging Commences

n0kto Lead Judge 7 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.