Missing fees allow cheap griefing attacks that lead to DoS

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXDeposit.sol#L117

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXDeposit.sol#L115C5-L115C14

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXChecks.sol#L59-L60

Summary

The protocol has chosen a design pattern which does not allow two users at the same time to interact with the system as every time a user deposits or withdraws funds a 2-step process begins which interacts with GMX and only after this process is closed, another user is allowed to start a new process. This design pattern can be abused as griefing attack by front running all user calls with a small deposit, or withdraw call, to DoS the user's call. As the protocol is deployed on L2 blockchains with low transaction fees and does not take fees on depositing or withdrawing funds, this DoS griefing attack is cheap and can be scaled to a point where nobody is able to interact with the system.

Vulnerability Details

The design pattern of the system which leads to this possibility is the status variable.

The flow for such a griefing attack would look like the following:

• The system's status is Open
• User wants to deposit or withdraw and creates a transaction to do so
• Attacker front runs the call of the user and deposit or withdraw a small amount of funds (Systems status changes to Deposit or Withdraw)
• User's call gets reverted as the check if the system's status is Open reverts

Deposit function calls beforeDepositChecks and updates the status to Deposit:

function deposit(
  GMXTypes.Store storage self,
  GMXTypes.DepositParams memory dp,
  bool isNative
) external {
	...
	GMXChecks.beforeDepositChecks(self, _dc.depositValue);

  self.status = GMXTypes.Status.Deposit;
	...
}

The beforeDepositChecks function reverts if the current status is not Open:

function beforeDepositChecks(
  GMXTypes.Store storage self,
  uint256 depositValue
) external view {
  if (self.status != GMXTypes.Status.Open)
    revert Errors.NotAllowedInCurrentVaultStatus();
	...
}

The same pattern is implemented in the withdraw flow.

Impact

DoS of the whole system for all depositors.

Tools Used

Manual Review

Recommendations

Implement fees, for depositing and withdrawing, to increase the costs of such a griefing attack, or rethink the status architecture.