handsomegiraffe - [M-01] Incorrect refund of execution fee to user

[sherlock-admin]

handsomegiraffe

medium

[M-01] Incorrect refund of execution fee to user

Summary

During execution of Deposits, Withdrawals and Orders, users are refunded part of the executionFee after accounting for gasUsed during the transaction. In the codebase, an incorrect value of startingGas is used to calculate the gasUsed, resulting in users getting less than what they should be refunded.

Vulnerability Detail

Vulnerability exists in DepositHandler.sol, WithdrawalHandler.sol and OrderHandler.sol. Using DepositHandler.sol as an example:

((1) In line 94 of DepositHandler.sol, Order Keepers call executeDeposit() and startingGas is forwarded to an external call _executeDeposit.
(2) In ExecuteDepositUtils.sol, _executeDeposit further calls GasUtils.payExecutionFee(... params.startingGas.
(3) Then in GasUtils.sol, payExecutionFee() calculates gasUsed = startingGas - gasleft();
(4) gasUsed is used to calculate executionFeeForKeeper, and after paying the fee to keeper, the remainder of executionFee (previously paid by user) is refunded to the user

The issue lies with (1) where startingGas is passed into _executeDeposit and assumed to be all remaining gas left. EIP-150 defines the "all but one 64th" rule, which states that always at least 1/64 of the gas still not used for this transaction cannot be sent along. Therefore, in (3) gasUsed is overstated by 1/64 and the refund back to user in (4) is incorrect (less than what user should get back).

Proof of Concept

In the test above, it is demonstrated that external function calls are forwarded with only 63/64 of the remaining gas. A separate internal function call used to demonstrate the difference in gas costs.

Impact

GMX Users will receive an incorrect refund from the execution fee and will be overpaying for deposit, withdraw and order executions.

Code Snippet

https://github.com/gmx-io/gmx-synthetics/blob/b1557fa286c35f54c65a38a7b57baf87ecad1b5b/contracts/exchange/DepositHandler.sol#L100
https://github.com/gmx-io/gmx-synthetics/blob/b1557fa286c35f54c65a38a7b57baf87ecad1b5b/contracts/exchange/WithdrawalHandler.sol#L130
https://github.com/gmx-io/gmx-synthetics/blob/b1557fa286c35f54c65a38a7b57baf87ecad1b5b/contracts/exchange/OrderHandler.sol#L174

Tool used

Hardhat
Manual Review

Recommendation

In DepositHandler.sol, for executeDeposit it is recommended that startingGas() is calculated after the external call is made.

Alternatively, in GasUtils.sol, gasUsed could be computed with 63/64 of startingGas, in order to obtain the correct refund amount to the user. This would also apply to Withdraw and Order executions which have similar code flows.

[xvi10]

this is a valid concern but we do not think this requires a change in the contracts, startingGas is measured after withOraclePrices, so there will be some extra gas consumed for that call, additionally the keeper should be incentivised with a small fee to execute the requests, the value for Keys.EXECUTION_GAS_FEE_BASE_AMOUNT can be adjusted to account for these

[Jiaren-tang]

Escalate for 10 USDC. I do not recommend escalating this issue for a reward of 10 USDC, as the impact is too trivial. According to the protocol's comments, not forwarding 1/64th of the gas to compensate for the execution fee does not block order flow execution, and Keys.EXECUTION_GAS_FEE_BASE_AMOUNT can be configured to easily solve this issue.

Furthermore, based on Sherlock's severity guide:

https://docs.sherlock.xyz/audits/judging/judging

a Medium severity rating should only be given if there is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

Based on the protocol's comment, this issue is clearly considered an acceptable risk by a reasonable protocol team. Therefore, I do not believe it meets the criteria for a Medium severity rating.
@xvi10 @IllIllI000

[sherlock-admin]

Escalate for 10 USDC. I do not recommend escalating this issue for a reward of 10 USDC, as the impact is too trivial. According to the protocol's comments, not forwarding 1/64th of the gas to compensate for the execution fee does not block order flow execution, and Keys.EXECUTION_GAS_FEE_BASE_AMOUNT can be configured to easily solve this issue.

Furthermore, based on Sherlock's severity guide:

https://docs.sherlock.xyz/audits/judging/judging

a Medium severity rating should only be given if there is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

Based on the protocol's comment, this issue is clearly considered an acceptable risk by a reasonable protocol team. Therefore, I do not believe it meets the criteria for a Medium severity rating.
@xvi10 @IllIllI000

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.

[giraffe0x]

Escalate for 10 USDC. I opine that the issue is valid and should be retained for award.

1. The protocol has acknowledged this as a valid issue.
2. While no code change is needed, the protocol said they can adjust Keys.EXECUTION_GAS_FEE_BASE_AMOUNT to account for this -- indicating there was an issue in the first place which needs to be fixed
3. Keeper is already incentivsed through a defined fee see link, therefore this additional gas that the user has to pay for is unintended and should be remedied. Given the popularity of the protocol, the aggregate amount of gas users will be overpaying for is significant

[sherlock-admin]

Escalate for 10 USDC. I opine that the issue is valid and should be retained for award.

1. The protocol has acknowledged this as a valid issue.
2. While no code change is needed, the protocol said they can adjust Keys.EXECUTION_GAS_FEE_BASE_AMOUNT to account for this -- indicating there was an issue in the first place which needs to be fixed
3. Keeper is already incentivsed through a defined fee see link, therefore this additional gas that the user has to pay for is unintended and should be remedied. Given the popularity of the protocol, the aggregate amount of gas users will be overpaying for is significant

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]

I agree with the second escalation for its stated reasons. In addition, if the swap path is long enough, the base fee may not be large enough to cover the number of swaps

[hrishibhat]

Escalation accepted

Considering this issue as valid based on Sponsor, 2nd Escalation and Lead Watson comments.

[sherlock-admin]

Escalation accepted

Considering this issue as valid based on Sponsor, 2nd Escalation and Lead Watson comments.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[xvi10]

Fix in https://github.com/gmx-io/gmx-synthetics/pull/119