Withdraw function provides more funds to withdrawer

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWithdraw.sol#L67-L69

https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWithdraw.sol#L101

Summary

Because mintFee is called later then user's supply ratio calculation, then this ratio is bigger than in reality and user receives more funds.

Vulnerability Details

When user calls withdraw, then he provides shareAmt, which is amount of GMXVault shares to withdraw. Then function calculates user's supply ratio as shareAmt / totalSupply. Then according to that ratio it's possible to understand how many GMX lpAmt should be withdrawn for user.

Later this function will do one more thing: it will mint fee for protocol. This function will increase totalSupply. Protocol accrues fee for each second and in order to get correct amount you should use not totalSupply, but totalSupply + GMXReader.pendingFee.

As result, totalSupply is less than in reality and user receives bigger ration and withdraws more assets.

Exactly same problem has GMXEmergency.emergencyWithdraw function. And overall, incorrect totalSupply function can create integration issues for other protocols.

Impact

User withdraws more than should.

Tools Used

VsCode

Recommendations

Do minting before calculation of supply ratio. Or better override totalSupply function to return supply with pending fees.