Relevant GitHub Links
Summary
When a deposit fails, the contract can become stuck in a
deposit_failed status due to improper handling of debt repayment by swapping through the swapTokensForExactTokens() function.which leads to gas losses for keeper attempting to handle that and puts user deposits at risk.Vulnerability Details
- In case of a user making a deposit to the
strategy, it will create a deposit inGMX. After a successful deposit,GMXwill call the callback functionafterDepositExecution, and the callback function will callprocessDeposit.
- If the
processDeposit()fails in thetrycall for any reason, the function willcatchthat and set the status todeposit_failed. An event will be emitted so the keeper can handle it.
plain textfunction processDeposit(GMXTypes.Store storage self) external { // some code .. >> try GMXProcessDeposit.processDeposit(self) { // ..more code } catch (bytes memory reason) { >> self.status = GMXTypes.Status.Deposit_Failed; emit DepositFailed(reason); } }
- The keeper calls the function processDepositFailure(). This function initiates a
requestWithdrawtoGMXto remove the liquidity added by the user deposit (+ the borrowed amount).
- After executing the
removeLiquidity, the callback functionafterWithdrawalExecutionis triggered. and since the status isdeposit_failed, it invokes the functionprocessDepositFailureLiquidityWithdrawal.
- In
processDepositFailureLiquidityWithdrawal, it first checks if a swap is necessary. If required, it swaps tokens to repay the debt.
plain text>> (bool _swapNeeded, address _tokenFrom, address _tokenTo, uint256 _tokenToAmt) = GMXManager.calcSwapForRepay(self, _rp); if (_swapNeeded) { ISwap.SwapParams memory _sp; _sp.tokenIn = _tokenFrom; _sp.tokenOut = _tokenTo; _sp.amountIn = IERC20(_tokenFrom).balanceOf(address(this)); _sp.amountOut = _tokenToAmt; _sp.slippage = self.minSlippage; _sp.deadline = block.timestamp; >> GMXManager.swapTokensForExactTokens(self, _sp); }
- The problem arises if the swap revert if the
tokenInbalance is insufficient to cover the_amountOutof_tokenOut, leading to a failed swap since the swap function isswapTokensForExactTokens. Consequently, the status remainsdeposit_failedand the callback revet.
Note: The swap can fail for various reasons.
- In this scenario, the keeper can only invoke the
processDepositFailure()function again. During the second call, it directly triggersprocessDepositFailureLiquidityWithdrawalsince thelptokens for the failed deposit has already been withdrawn.
plain textfunction processDepositFailure(GMXTypes.Store storage self, uint256 slippage, uint256 executionFee) external { GMXChecks.beforeProcessAfterDepositFailureChecks(self); GMXTypes.RemoveLiquidityParams memory _rlp; // If current gmx LP amount is somehow less or equal to amount before, we do not remove any liquidity if (GMXReader.lpAmt(self) <= self.depositCache.healthParams.lpAmtBefore) { >> processDepositFailureLiquidityWithdrawal(self); //... more code }}
- The swap will always revert because the contract's balance of
tokenInwill never be sufficient to cover the_amountOutof_tokenOut. Consequently, the status remains stuck atdeposit_failed.
Impact
- The strategy remains stuck at the
deposit_failedstatus, halting any further interactions with the protocol.
- Keepers lose gas for each call to
processDepositFailure().
- Users may lose their deposits.
Tools Used
vs code
manual review
Recommendations
- Utilize
swapExactTokensForTokensand swap the remaining tokens fromtokenInafter substracting debt need to be repaid of this token.fortokenOut.
- Implement safeguards to calculate the appropriate amount for swapping, avoiding potential reverting transactions. Here's an example of how to calculate the swap amount:
plain textif (rp.repayTokenAAmt > self.tokenA.balanceOf(address(this))) { // If more tokenA is needed for repayment if(rp.repayTokenBAmt < self.tokenB.balanceOf(address(this))){ _tokenToAmt = self.tokenB.balanceOf(address(this)) - rp.repayTokenBAmt; _tokenFrom = address(self.tokenB); _tokenTo = address(self.tokenA); } }