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

Inadequate Error Handling in Token Transfers

verbotenviking

Summary

In the PerpetualVault contract, when transferring tokens to a user during withdrawal fails, the contract silently sends the tokens to the treasury address instead. This implementation lacks proper error handling and recovery mechanisms, potentially resulting in permanent loss of user funds without adequate notification or recourse.

Vulnerability Details

The issue is located in the _transferToken function in PerpetualVault.sol (lines 1752-1758):

try collateralToken.transfer(depositInfo[depositId].recipient, amount - fee) {}
catch {
collateralToken.transfer(treasury, amount - fee);
emit TokenTranferFailed(depositInfo[depositId].recipient, amount - fee);
}

When a token transfer to a user fails, the contract catches the exception and performs two actions:

  1. Immediately attempts to transfer the same amount to the treasury address

  2. Emits a TokenTranferFailed event

There are several issues with this approach:

  • No error handling exists for the fallback transfer to the treasury

  • No mechanism exists for users to recover their funds if the transfer fails

  • The failure is only logged through an event, which users might not monitor

  • The implementation assumes that any transfer failure is permanent, when it might be temporary

Impact

If a token transfer to a user fails (which could happen for various reasons including temporary contract issues, gas limitations, or receiving contract reverts), the user's funds will be permanently sent to the treasury with no automated recovery mechanism. This could result in permanent loss of user funds if the treasury is unresponsive or unaware of the need to manually return these funds. Furthermore, if the fallback transfer to the treasury also fails, the funds could become stuck in the contract indefinitely.

Tools Used

Manual code review

Recommendations

  1. Implement a proper recovery mechanism that allows users to claim their funds later if a transfer fails:

mapping(address => uint256) public failedTransferCredits;
​
function _transferToken(uint256 depositId, uint256 amount) internal {
uint256 fee;
if (amount > depositInfo[depositId].amount) {
fee = (amount - depositInfo[depositId].amount) * governanceFee / BASIS_POINTS_DIVISOR;
if (fee > 0) {
collateralToken.safeTransfer(treasury, fee);
}
}
address recipient = depositInfo[depositId].recipient;
try collateralToken.transfer(recipient, amount - fee) {
// Transfer succeeded
} catch {
// Record failed transfer for later withdrawal
failedTransferCredits[recipient] += amount - fee;
emit TokenTranferFailed(recipient, amount - fee);
}
}
​
// Add a function for users to claim their funds later
function withdrawFailedTransfer() external {
uint256 amount = failedTransferCredits[msg.sender];
require(amount > 0, "No failed transfers");
failedTransferCredits[msg.sender] = 0;
// Using safeTransfer with require to ensure exceptions are visible
require(collateralToken.transfer(msg.sender, amount), "Transfer failed");
emit FailedTransferClaimed(msg.sender, amount);
}

  1. For the treasury transfer, use safeTransfer to ensure any errors are captured and properly handled.

  2. Consider adding additional logging or notifications to alert system administrators when transfers fail.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
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: Design choice
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!