Relevant GitHub Links
Summary
The state of the vault, in the context of the steadefi strategy vaults is one of the most critical aspect of the operations of the protocol, as they determine what the vault is actually doing at a particular time, also this vault state determines the success of the after checks, after and before every type of transactions, to determine if the transactions should fail or succeed. So if the state of the vault should be stuck it could be fatal to the functionality of the protocol. And this is exactly what happened when a Lending vault can indirectly hold all vault Interacting with it a halt.
Vulnerability Details
This issue will happen at some specific combination of actions from both the Strategy Vaults and the Lending Vault, causing the state of the Vault to be stuck, let's explore this further with an illustration of how the event can affect the state
User tries to withdraw their tokens from the GMX Vaults, and for some reason like checking the health of the vaults, the action fails, and it goes into the processWithdrawalFailure Flow
In the failure withdrawal failure processing flow, it immediately tries to borrow back the same amount it just repaid back to the lending vaults, to roll back its actions and return the total assets back to GMX
plain text// Re-borrow assets based on the repaid amount GMXManager.borrow( self, self.withdrawCache.repayParams.repayTokenAAmt, self.withdrawCache.repayParams.repayTokenBAmt );
At this point the state of the vault is GMXTypes.Status.Withdraw_Failed, to show that the vault is in a failed withdrawal state
Now This exact point is where the Stuck state can happen
When the function tries to borrow again from the vault, they are some unknown factor that could determine if that action will succeed,
First, is actually if the Lending Vault is in a paused state, or some kind of restriction placed on borrowing from it.
But this is exactly what made this Vulnerability Happen, The fact that a user could repay a loan to a restricted/paused vault but cannot borrow back from them, put a full stop to the actions the vault wants to roll back.
plain textfunction repay(uint256 repayAmt) external nonReentrant {
When this happens the function reverts, because lack of access to borrow, but the whole step does not, to roll back the state of the vault, because this is a two step transaction
plain textfunction borrow(uint256 borrowAmt) external nonReentrant whenNotPaused onlyBorrower {
Which Cause a Stuck State, But this stuck state is more fatal, as the different Vaults that borrow from this particular Lending Vaults can be held By the Leverage Vaults Ability to resume/Remove the restrictions as to complete their functions.
Impact
The Impact of this Kind of vulnerability is pretty clear, the fact that the state determines the actions that could be performed in this vault, means it not functioning as expected could lead to a complete halt, in the activities of the vault
Tools Used
Manual Review
Recommendations
My Recommendation is that the code should be refactored in a way that the users can still borrow back the same money they just made, this will take into account the two step transactions it takes to complete a single action. But this kind of decision also needs to take in the design choices of the Lending Vault into consideration to achieve a clear path to mitigation.