Steadefi
Steadefi
DeFiHardhatFoundryOracle
35,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

GMXManager.swapExactTokensForTokens and GMXManager.addLiquidity, without using the "Checks-Effects-Interactions" pattern, this could lead to a potential drain/loss of fund

0xVinylDavyl

Summary

The GMXCompound.sol contract contains a vulnerability related to reentrancy risk due to its interaction with external contracts. The code doesn't follow the "Checks-Effects-Interactions" pattern, which can create reentrancy risks.

Vulnerability Details

GMXCompound.sol contract file interacts with external contracts, such as GMXManager.swapExactTokensForTokens and GMXManager.addLiquidity, without using the "Checks-Effects-Interactions" pattern. This pattern ensures that state changes are made before interacting with external contracts. The absence of this pattern can potentially create reentrancy risks.

In the GMXCompound.sol contract, the vulnerability related to reentrancy risk is present in the compound function. Identified the code block where this vulnerability occurs:

function compound(
GMXTypes.Store storage self,
GMXTypes.CompoundParams memory cp
) external {
// Transfer any tokenA/B from trove to vault
if (self.tokenA.balanceOf(address(self.trove)) > 0) {
self.tokenA.safeTransferFrom(
address(self.trove),
address(this),
self.tokenA.balanceOf(address(self.trove))
);
}
if (self.tokenB.balanceOf(address(self.trove)) > 0) {
self.tokenB.safeTransferFrom(
address(self.trove),
address(this),
self.tokenB.balanceOf(address(self.trove))
);
}
​
uint256 _tokenInAmt = IERC20(cp.tokenIn).balanceOf(address(this));
​
// Only compound if tokenIn amount is more than 0
if (_tokenInAmt > 0) {
// Apply state changes here
​
self.refundee = payable(msg.sender);
self.compoundCache.compoundParams = cp;
// ...
​
// Now interact with external contracts
ISwap.SwapParams memory _sp;
// ...
​
GMXManager.swapExactTokensForTokens(self, _sp);
// ...
​
GMXTypes.AddLiquidityParams memory _alp;
// ...
​
// This is where the external interaction happens before state changes are finalized
self.compoundCache.depositKey = GMXManager.addLiquidity(self, _alp);
}
}

The state changes are made within the same block as the interaction with an external contract (GMXManager.addLiquidity). This can create a reentrancy risk because the state changes should typically be made before interacting with external contracts, following the "Checks-Effects-Interactions" pattern to minimize reentrancy risks. To mitigate this vulnerability, the state changes should be finalized before calling external contracts, as recommended in the previous response.

Impact

Reentrancy attacks can result in unexpected behavior and potential loss of funds. An attacker could exploit this vulnerability to call functions from external contracts that modify the state of the GMXCompound contract before its state changes are applied.

Tools Used

Manual

Recommendations

Recommend to follow the "Checks-Effects-Interactions" pattern when interacting with external contracts. This involves ensuring that state changes are made before any interactions with external contracts. Additionally, consider using the "ReentrancyGuard" pattern to further mitigate reentrancy risks.
By applying the "Checks-Effects-Interactions" pattern, the contract ensures that state changes are made before any external interactions, reducing reentrancy risks. This pattern should be consistently applied throughout the contract's functions.

Showing how the code can be modified to follow the "Checks-Effects-Interactions" pattern:

function compound(
GMXTypes.Store storage self,
GMXTypes.CompoundParams memory cp
) external {
// Transfer any tokenA/B from trove to vault
if (self.tokenA.balanceOf(address(self.trove)) > 0) {
self.tokenA.safeTransferFrom(
address(self.trove),
address(this),
self.tokenA.balanceOf(address(self.trove))
);
}
if (self.tokenB.balanceOf(address(self.trove)) > 0) {
self.tokenB.safeTransferFrom(
address(self.trove),
address(this),
self.tokenB.balanceOf(address(self.trove))
);
}
​
uint256 _tokenInAmt = IERC20(cp.tokenIn).balanceOf(address(this));
​
// Only compound if tokenIn amount is more than 0
if (_tokenInAmt > 0) {
// Apply state changes here
​
self.refundee = payable(msg.sender);
self.compoundCache.compoundParams = cp;
// ...
​
// Now interact with external contracts
ISwap.SwapParams memory _sp;
// ...
​
GMXManager.swapExactTokensForTokens(self, _sp);
// ...
​
GMXTypes.AddLiquidityParams memory _alp;
// ...
​
// Ensure state changes are applied before interacting with external contracts
self.compoundCache.depositKey = GMXManager.addLiquidity(self, _alp);
}
}
Updates

Lead Judging Commences

hans Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Lack of quality
0xVinylDavyl Submitter
almost 2 years ago
hans Auditor
almost 2 years ago
hans Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

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