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

re-entrency possible on processWithdraw since external call is made before burning user's shares in Vault

Citris

Summary

re-entrency possible on processWithdraw since external call is made before burning user's shares in Vault

if (self.withdrawCache.withdrawParams.token == address(self.WNT)) {
self.WNT.withdraw(self.withdrawCache.tokensToUser);
@>audit transfer ETH and call (bool success, ) = self.withdrawCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
} else {
// Transfer requested withdraw asset to user
IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(
self.withdrawCache.user,
self.withdrawCache.tokensToUser
);
}
​
// Transfer any remaining tokenA/B that was unused (due to slippage) to user as well
self.tokenA.safeTransfer(self.withdrawCache.user, self.tokenA.balanceOf(address(this)));
self.tokenB.safeTransfer(self.withdrawCache.user, self.tokenB.balanceOf(address(this)));
​
// Burn user shares
@> burn is after self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);

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

Since the function is only accessible by keeper (likely a router), which from the example of the mockRouter, would bundle the withdraw and "afterWithdrawalExecution" together. However since the router is out-of-scope, and there is still a possible chance that the user can make use of the router to re-enter into the function (without re-entrency lock), and be able to drain more fund that he actually deserves. This is submitted as a medium risk.

Vulnerability Details

Impact

drain of user funds.

Tools Used

Recommendations

burn user's share first, before executing external call at the end.

Updates

Lead Judging Commences

hans Auditor
over 1 year ago
hans Auditor
over 1 year ago
hans Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Reentrancy by keepers

Impact: High Likelihood: Low The only possible external caller is the keepers. But this is still a vulnerability and it is strongly recommended to implement CEI pattern. Given the limited impact, similar issues (reentrancy by keepers) are grouped.

nmirchev8 Auditor
over 1 year ago
hans Auditor
over 1 year ago
blckhv Auditor
over 1 year ago
Steadefi Lead Judge
over 1 year ago
hans Auditor
over 1 year ago
ElHaj Auditor
over 1 year ago
hans Auditor
over 1 year ago
hans Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Reentrancy by keepers

Impact: High Likelihood: Low The only possible external caller is the keepers. But this is still a vulnerability and it is strongly recommended to implement CEI pattern. Given the limited impact, similar issues (reentrancy by keepers) are grouped.

Support

FAQs

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