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,GMX
will call the callback functionafterDepositExecution
, and the callback function will callprocessDeposit
.
- If the
processDeposit()
fails in thetry
call for any reason, the function willcatch
that 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
requestWithdraw
toGMX
to remove the liquidity added by the user deposit (+ the borrowed amount).
- After executing the
removeLiquidity
, the callback functionafterWithdrawalExecution
is 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
tokenIn
balance is insufficient to cover the_amountOut
of_tokenOut
, leading to a failed swap since the swap function isswapTokensForExactTokens
. Consequently, the status remainsdeposit_failed
and 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 triggersprocessDepositFailureLiquidityWithdrawal
since thelp
tokens 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
tokenIn
will never be sufficient to cover the_amountOut
of_tokenOut
. Consequently, the status remains stuck atdeposit_failed
.
Impact
- The strategy remains stuck at the
deposit_failed
status, 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
swapExactTokensForTokens
and swap the remaining tokens fromtokenIn
after 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); } }