Steadefi
Steadefi
DeFiHardhatFoundryOracle
35,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Lack of a logic to refund the `unused native gas` (`excess` execution fees) to the depositor

0xmuxyz

Summary

According to the "GMX v2 High Level Sequence" in the Technical Documentation, once adding liquidity to a GM Pool would be successful, unused native gas (excess execution fee) back to the GMXVault and then it would be refunded to the depositor during the callback process of deposit.

Based to the references above, a logic to refund the unused native gas to the depositor after sending it back to the GMXVault from the GM Pool is supposed to be implemented.

However, through the callback process of deposit above, there is no logic to refund the unused native gas to the depositor after sending it back to the GMXVault from the GM Pool.

This lead to that the unused native gas (the excess execution fees) would be stuck in the GMXVault and never be refunded to the depositor.

Vulnerability Details

When the GMXVault#deposit() would be called, tokenA/tokenB would be added to a GM Pool (in GMX) for liquidity and receive LP tokens (GM tokens) via the GMXWorker#addLiquidity().

Within the GMXWorker#addLiquidity(), each parameter would be set to the _cdp (IExchangeRouter.CreateDepositParams). The executionFee would be assigned as a parameter into the _cdp in order to pay for a gas fee in the GM Pool to add liquidity.

Then, the ExchangeRouter#createDeposit() would be called with the parameters (_cdp) like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWorker.sol#L101
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWorker.sol#L105

/**
* @dev Add strategy's tokens for liquidity and receive LP tokens
* @param self Vault store data
* @param alp GMXTypes.AddLiquidityParams
* @return depositKey Hashed key of created deposit in bytes32
*/
function addLiquidity(
GMXTypes.Store storage self,
GMXTypes.AddLiquidityParams memory alp
) external returns (bytes32) {
...
// Create deposit
IExchangeRouter.CreateDepositParams memory _cdp =
IExchangeRouter.CreateDepositParams({
receiver: address(this),
callbackContract: self.callback,
uiFeeReceiver: self.refundee,
market: address(self.lpToken),
initialLongToken: address(self.tokenA),
initialShortToken: address(self.tokenB),
longTokenSwapPath: new address[](0),
shortTokenSwapPath: new address[](0),
minMarketTokens: alp.minMarketTokenAmt,
shouldUnwrapNativeToken: false,
executionFee: alp.executionFee, ///<-------------- @audit
callbackGasLimit: 2000000
});
​
return self.exchangeRouter.createDeposit(_cdp); ///<--------------- @audit
}

Once adding tokenA/tokeB to a GM Pool for liquidity would be successful, the callback process of deposit would be initiated via the GMXCallback#afterDepositExecution().
Within the GMXCallback#afterDepositExecution(), the GMXVault#processDeposit() would be called like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXCallback.sol#L68

/**
* @notice Process vault after successful deposit execution from GMX
* @dev Callback function for GMX handler to call
* @param depositKey bytes32 depositKey hash of deposit created
*/
function afterDepositExecution(
bytes32 depositKey,
IDeposit.Props memory /* depositProps */,
IEvent.Props memory /* eventData */
) external onlyController {
GMXTypes.Store memory _store = vault.store();
​
if (
_store.status == GMXTypes.Status.Deposit &&
_store.depositCache.depositKey == depositKey
) {
vault.processDeposit(); ///<---------- @audit
} else if (
...

Within the GMXVault#processDeposit(), the GMXDeposit#processDeposit() would be called like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXVault.sol#L363

/**
* @notice Post deposit operations if adding liquidity is successful to GMX
* @dev Should be called only after deposit() / depositNative() is called
* @dev Should be called by approved vault's Callback or approved Keeper
*/
function processDeposit() external onlyKeeper {
GMXDeposit.processDeposit(_store); ///<---------- @audit
}

Within the GMXDeposit#processDeposit(), the GMXProcessDeposit#processDeposit() would be called like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXDeposit.sol#L170

/**
* @notice @inheritdoc GMXVault
* @param self GMXTypes.Store
*/
function processDeposit(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessDepositChecks(self);
​
// We transfer the core logic of this function to GMXProcessDeposit.processDeposit()
// to allow try/catch here to catch for any issues or any checks in afterDepositChecks() failing.
// If there are any issues, a DepositFailed event will be emitted and processDepositFailure()
// should be triggered to refund assets accordingly and reset the vault status to Open again.
try GMXProcessDeposit.processDeposit(self) { ///<---------- @audit
// Mint shares to depositor
self.vault.mint(self.depositCache.user, self.depositCache.sharesToUser); ///<---------- @audit
​
self.status = GMXTypes.Status.Open;
...
} catch (bytes memory reason) {
self.status = GMXTypes.Status.Deposit_Failed;
...
}
}

Within the GMXProcessDeposit#processDeposit(), the GMXChecks#afterDepositChecks() would be called like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXProcessDeposit.sol#L32

/**
* @notice @inheritdoc GMXVault
* @param self GMXTypes.Store
*/
function processDeposit(
GMXTypes.Store storage self
) external {
self.depositCache.healthParams.equityAfter = GMXReader.equityValue(self);
​
self.depositCache.sharesToUser = GMXReader.valueToShares(
self,
self.depositCache.healthParams.equityAfter - self.depositCache.healthParams.equityBefore,
self.depositCache.healthParams.equityBefore
);
​
GMXChecks.afterDepositChecks(self); ///<---------- @audit
}

Within the GMXChecks#afterDepositChecks(), some states after deposit would be checked like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXChecks.sol#L99-L118

/**
* @notice Checks after deposit
* @param self GMXTypes.Store
*/
function afterDepositChecks(
GMXTypes.Store storage self
) external view {
// Guards: revert if lpAmt did not increase at all
if (GMXReader.lpAmt(self) <= self.depositCache.healthParams.lpAmtBefore)
revert Errors.InsufficientLPTokensMinted();
​
// Guards: check that debt ratio is within step change range
if (!_isWithinStepChange(
self.depositCache.healthParams.debtRatioBefore,
GMXReader.debtRatio(self),
self.debtRatioStepThreshold
)) revert Errors.InvalidDebtRatio();
​
// Slippage: Check whether user received enough shares as expected
if (
self.depositCache.sharesToUser <
self.depositCache.depositParams.minSharesAmt
) revert Errors.InsufficientSharesMinted();
}

According to the official GMX v2 documentation, any excess execution fee is sent back to the deposit's account after adding liquidity to a GM Pool would be successful like this:

executionFee: The amount of native token that is included for the execution fee, e.g. on Arbitrum this would be ETH, this is the maximum execution fee that keepers can use to execute the deposit. When the deposit is executed, any excess execution fee is sent back to the deposit's account. Please see the Execution Fee section for more details.

Also, according to the "GMX v2 High Level Sequence" in the Technical Documentation, unused native gas back to the GMXVault and then it would be refunded to the depositor like this:

For example, when a Depositor is depositing to a Vault which will add liquidity to GMX to ETH-USDC GM pool:
...

  • GMX keepers will execute the 2nd transaction:

  • Swaps deposit tokens for LP (ETH-USDC GM) tokens

  • Sends LP tokens to Vault

  • Calls afterDepositExecution() to callback contract address of Vault (if successful deposit)

    • Vault computes how many Vault share tokens to mint and send to Depositor

  • Refund unused native gas back to Vault ///<-------------------- @audit

    • Vault refunds unused gas tokens to Depositor ///<-------------------- @audit

Based to the references above, a logic to refund the unused native gas to the depositor after sending it back to the GMXVault from the GM Pool is supposed to be implemented.

However, through the callback process of deposit above, there is no logic to refund the unused native gas to the depositor after sending it back to the GMXVault from the GM Pool.

This lead to that the unused native gas (the excess execution fees) would be stuck in the GMXVault and never be refunded to the depositor.

Note
The callback process of withdrawal has the same vulnerability.

Impact

This lead to that the unused native gas (the excess execution fees) would be stuck in the GMXVault and never be refunded to the depositor.

Tools Used

  • Foundry

Recommendations

Within the callback process of deposit and withdrawal, consider adding a logic to refund the unused native gas (the excess execution fees) to the depositor after sending it back to the GMXVault from the GM Pool.

Updates

Lead Judging Commences

hans Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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