M-31. Setting minSharesAmt high always leads to processDeposit failure

Submitted by innertia.

Relevant GitHub Links

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

Summary

At the deposit stage, large minSharesAmt are not checked and the status of the contract is changed to Deposit. However, it will be checked at the next stage of processDeposit, which can lead to a failure without fail. Since the status change affects the entire contract, a large number of malicious Deposits can disrupt the normal progress of business.

Vulnerability Details

The uint256 minSharesAmt in DepositParams can be determined by the user at Deposit time. By setting this to a large value, while successfully changing state to Deposit, the following checks cannot be broken through in subsequent phases, and the transaction will fail.

plain text
if (
self.depositCache.sharesToUser <
self.depositCache.depositParams.minSharesAmt
) revert Errors.InsufficientSharesMinted();
}

Impact

Disrupts normal business operations by issuing malicious Deposits in large quantities

Tools Used

Manual

Recommendations

Set a realistic upper limit on minSharesAmt.