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

Malicous keepers are capable of stealing funds from the protocol.

NeverGonnaGiveYulUp

Summary

While keepers are currently centralized the intention has been stated to make them decentralized down the road (Link). There is an attack vector that allows a keeper to steal funds from the protocol.

Vulnerability Details

In GMXVault:compound There is no input validation for GMXTypes.CompoundParams. The malicous keeper can pass in any ERC20 token with 0 balance in the vault, which will bypass the logic in the if-statement. This will simply cause GMXVault:compound to transfer tokens from the trove to the vault.

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));
​
if (_tokenInAmt > 0) {
...
}
}

Since bypassing the if-statement also bypasses the status check in beforeCompoundChecks, the transfer from the trove to the GMXVault can be executed at any time. If a malicious keeper times this compound call inbetween GMXWithdraw:withdraw and GMXWithdraw:processWithdraw he can transfer the tokens from the trove to himself as processWithdraw transfers all tokens A/B of the contract to the user:

// 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)));

Impact

A malicous keeper is capable of stealing funds from the trove

Tools Used

Manual Review and Discussion with Team on Discord

Recommendations

Options:

  1. remove the if (_tokenInAmt > 0) check

  2. Perform the status check that happens in beforeCompoundChecks at the beginning of GMXCompound:compound

Updates

Lead Judging Commences

hans Auditor
almost 2 years ago
hans Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Spearfish5609 Submitter
almost 2 years ago
hans Auditor
almost 2 years ago
hans Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

Trove assets can remain in the vault and used by other users

Support

FAQs

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