Open sherlock-admin opened 1 year ago
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
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
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.
Escalate for 10 USDC. I opine that the issue is valid and should be retained for award.
Escalate for 10 USDC. I opine that the issue is valid and should be retained for award.
- The protocol has acknowledged this as a valid issue.
- 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
- 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.
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
Escalation accepted
Considering this issue as valid based on Sponsor, 2nd Escalation and Lead Watson comments.
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.
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 forgasUsed
during the transaction. In the codebase, an incorrect value ofstartingGas
is used to calculate thegasUsed
, 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()
andstartingGas
is forwarded to an external call_executeDeposit
. (2) In ExecuteDepositUtils.sol,_executeDeposit
further callsGasUtils.payExecutionFee(... params.startingGas
. (3) Then in GasUtils.sol,payExecutionFee()
calculatesgasUsed = startingGas - gasleft();
(4)gasUsed
is used to calculateexecutionFeeForKeeper
, and after paying the fee to keeper, the remainder ofexecutionFee
(previously paid by user) is refunded to the userThe 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 thatstartingGas()
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.