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

Pausing contract in between user interactions (deposit) could result in lost funds)

nmirchev8

Summary

Impact Likelihood Overall
High Low Medium

The contract provide functionality to pause the vault in some emergency situation. The function doesn't check the state of the vault, because it is "emergency" and so it should be possible to trigger it any time. However, this could result in user loses, if it is called at the wrong time (in between user interactions like deposit or withdraw).

Vulnerability Details

The problem comes from the two way transaction process for GMX vaults and safety checks, which are implemented in the protocol.
For example if a user initiate a deposit transaction with $1000 USDC on vault X, the vault will add those tokens as liquidity to GMX and put the vault in state of "Deposit".
The problem arise from that it is possible to put the vault in state of "Paused", before the callback from GMX router, which would initiate vault tokens being minted and sent to the user. The callback will revert, because the state of the vault is "Paused":

function processDeposit(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessDepositChecks(self);
​
...
try GMXProcessDeposit.processDeposit(self) {

So the result is depositor looses his $1000, which benefit other depositors, whose vault token shares now worth more.

Impact

User's funds loss

Tools Used

Manual Review

Recommendations

Maybe consider medium state before "Paused", which would pass the check for processDeposit and officially pausing it after proceessDeposit.
Note that:

  • This is a solution if you want to maintain the opportunity to pause the protocol in any state and you should carefully examine all other related to deposit functions, such as "onCancelation", etc...

  • Here I provide just a basic example of how it could be achieved, but you should pay attention to the callbacks of deposit on the other states.

  • If you don't want to add more complexity, you could just prohibit pause to be executed, when the vault is in state "Deposit" and wait until it is safe to execute the function.

Example:

  • emergencyPause:

function emergencyPause(
GMXTypes.Store storage self
) external {
...code
if(self.status == GMXTypes.Status.Deposit){
self.status = GMXTypes.Status.PrePaused;
} else{
self.status = GMXTypes.Status.Paused;
}
emit EmergencyPause();
}

  • processDeposit:

function processDeposit(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessDepositChecks(self); // Allow state "PrePaused"
​
try GMXProcessDeposit.processDeposit(self) {
...code
if(self.status == GMXTypes.Status.PrePaused){
self.status = GMXTypes.Status.Paused
}
else{
self.status = GMXTypes.Status.Open;
}
emit DepositCompleted(
self.depositCache.user,
self.depositCache.sharesToUser,
self.depositCache.healthParams.equityBefore,
self.depositCache.healthParams.equityAfter
);
} catch (bytes memory reason) {
self.status = GMXTypes.Status.Deposit_Failed;
​
emit DepositFailed(reason);
}
}
Updates

Lead Judging Commences

hans Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

Not caching the previous state on emergencyPause

Impact: High Likelihood: Low/Medium Deposit or withdraw that were in progress will be ignored and cause fund loss. Because emergencyPause is only callable by keepers, Medium is the proper severity.

Support

FAQs

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