Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Suspension of the main workflow due to withdrawal of fees

druzh

Summary

A smart contract, in addition to tokens, accumulates ETH (for example, a user sends ether to pay for Keeper gas). The owner has the right to withdraw ETH from the contract. This is normal functionality, but you need to make sure that it does not allow you to output too much. Because Keeper's won't have enough gas.

Vulnerability Details

After checking the logic, I think that the contract stores the executionFee that users pay along with the deposit. These ethers are stored before the Keeper is used. Theoretically, the owner can, by an oversight of business logic, take these ethers before the Keeper's operations are performed, leaving the Keeper without gas. However, this situation is contrary to the interests of the project; in addition, if this happens, users will notice the non-fulfillment of their requests. I have not found any restrictions on the frequency of ETH withdrawal by the owner.

Withdrawal of ETH from contracts: The Owner can withdraw ether from GmxProxy. GmxProxy stores the ETH deposited by users as an executionFee. These ETH are needed to execute transactions on GMX. The documentation mentions, that the Owner can withdraw ETH, but it is worth doing it only if there is an excess.
Abuse: If the Owner withdraws ETH in advance, the Keeper's subsequent operations may fail due to lack of gas (contradicts the assumption: "Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc” - i.e. the contract expects that the executionFee covered). If the Owner withdraws the executionFee before execution, the user's deposit will remain unfulfilled, and the Keeper will receive the error LowerThanMinEth.

Impact

Business operation processing stops and the user has to pay an additional fee. There is no direct damage to the tokens, but this is a malfunction.
This behavior can be considered inappropriate – the Owner is acting against logic here. However, the protocol does not prohibit the Owner from doing so.

Tools Used

Foundry uint tests. In order to simplify the demonstration of PoC for CodeHawks (without creating a long code on the page), Withdrawal can be modified with the address of the recipient of the commissions. Next, keeper will do its job and get an error LowerThanMinEth.

function test_WithdrawETH() public {
address owner = PerpetualVault(vault).owner();
address keeper = PerpetualVault(vault).keeper();
address alice = makeAddr("alice");
​
depositFixture(alice, 2e10);
console.log(address(gmxProxy).balance);
​
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory swapData = new bytes[](2);
bytes memory paraSwapData = mockData.getParaSwapData(vault);
swapData[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
address[] memory gmxPath = new address[](1);
gmxPath[0] = address(0x70d95587d40A2caf56bd97485aB3Eec10Bee6336);
uint256 minOutputAmount = 1e10 * prices.shortTokenPrice.min / prices.longTokenPrice.min * 95 / 100; // 5% slippage
swapData[1] = abi.encode(PROTOCOL.GMX, abi.encode(gmxPath, 1e10, minOutputAmount));
​
// address treasury = makeAddr("treasury");
​
vm.prank(owner);
uint256 proxyBalance = GmxProxy(payable(gmxProxy)).withdrawEth();//.withdrawEth(treasury);
vm.stopPrank();
​
console.log(address(gmxProxy).balance);
vm.prank(keeper);
PerpetualVault(vault).run(true, true, prices, swapData);
GmxOrderExecuted(true);
​
// ...
​
}

Recommendations

This is solved by setting a minimum non-deductible balance or multi-stage withdrawal of funds, or I recommend linking this to, say, collecting fees once a month so that there is no temptation to stop the contract in the middle of work.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Support

FAQs

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

Give us feedback!