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

Unsafe and Outdated use of `transfer` for sending native ETH

fresh

Summary

In GmxProxy::withdrawEth-> the protocol allows the ownerto call the function to have ALL of the ETH in the contract transferred to them.

The problem is that the method of sending the ETH is not the most secure method and in an unexpected circumstance, the transfer can fail and the failure will not be handled.

Vulnerability Details

/**
* @notice Withdraws all ETH from the contract to the owner's address.
* @dev This function can only be called by the owner of the contract.
* @return The balance of ETH withdrawn from the contract.
*/
function withdrawEth() external onlyOwner returns (uint256) {
uint256 balance = address(this).balance;
-> payable(msg.sender).transfer(balance);
return balance;
}

It is best security practice to not use .transferwhen transferring native ETH.

Risks associated with .transfer:

  • limit of 2300 gas

  • FAILS SILENTLY

  • Transaction can fail but balancewill still be returned as if the transfer succeeded

There are no safety checks to ensure the transfer was successful either.

This .transferis also used in -PayExecutionFee-> which sends execution fee in ETH to the gmxProxy

function _payExecutionFee(uint256 depositId, bool isDeposit) internal {
uint256 minExecutionFee = getExecutionGasLimit(isDeposit) * tx.gasprice;
​
if (msg.value < minExecutionFee) {
revert Error.InsufficientAmount();
}
if (msg.value > 0) {
-> payable(address(gmxProxy)).transfer(msg.value);
depositInfo[depositId].executionFee = msg.value;
}
}

Impact

The transfer can fail and the ownerwont receive the ETH in that circumstance. The ETH will not be lost, it will still remain in the contract but balancewill be returned as if the transfer succeeded.

Because funds wont be lost, I labeled this as a low. But due to the nature of the action (withdrawing all of the ETH from the contract) - I believe it is very practical to consider using the best security practice for this type of transaction.

Tools Used

Manual Review

Recommendations

Use .callinstead of .transfer

Also, add a safety check ensuring the success of the transfer

(bool passed , ) = payable(msg.sender).call{value: balance}("");
require(passed, "Transfer to owner failed, ETH still in the contract");
Updates

Lead Judging Commences

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

Informational or Gas

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.

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 5 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

Informational or Gas

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.

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.