M-27. Missing fees allow cheap griefing attacks that lead to DoS

Submitted by Cosine.

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:

plain text
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:

plain text
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