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

Unhandled Failed Transfers in PerpetualVault._transferToken

scoolj

Summary

The _transferToken function in the PerpetualVault contract does not handle failed token transfers properly, leading to potential loss of user funds.

Vulnerability Details

The _transferToken function attempts to transfer tokens to the recipient. If the transfer fails, the function catches the error and redirects the funds to the treasury without explicit user consent. This can happen due to various reasons, such as the recipient address being a contract that does not implement the required interface or the recipient address being blacklisted.

RootCause:
The root cause of this issue is the lack of proper handling for failed token transfers in the _transferToken function. The function catches the error and redirects the funds to the treasury without notifying the user or providing a way to recover the funds.

Proof of Concept:
Here is a proof of concept demonstrating the issue:

  1. Deploy the PerpetualVault contract.

  2. Simulate a failed transfer by using a recipient address that is a contract without the required interface.

  3. Observe the funds being redirected to the treasury.

// Test function to demonstrate the issue
function test_UnhandledFailedTransfers() external {
address alice = makeAddr("alice");
address recipient = address(new NonReceiverContract()); // Contract that does not implement the required interface
depositFixture(alice, 1e10);
​
uint256[] memory depositIds = PerpetualVault(vault).getUserDeposits(alice);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(false);
vm.startPrank(alice);
vm.expectRevert(Error.InvalidUser.selector);
PerpetualVault(vault).withdraw{value: executionFee * tx.gasprice}(recipient, depositIds[0]);
vm.stopPrank();
​
// Check that the funds have been redirected to the treasury
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
uint256 treasuryBalance = collateralToken.balanceOf(PerpetualVault(vault).treasury());
assertTrue(treasuryBalance > 0);
}
​
// Dummy contract to simulate a failed transfer
contract NonReceiverContract {}

Impact

Users may lose access to their funds if the transfer fails and the funds are sent to the treasury. This can lead to unintended loss of funds for users, as they may not have intended for their funds to be sent to the treasury.

Tools Used

Manual

Recommendations

  1. Notify the User: Emit an event to notify the user of the failed transfer and provide instructions on how to recover their funds.

  2. Retry Mechanism: Implement a retry mechanism to allow users to retry the transfer or provide an alternative recipient address.

  3. User Consent: Require explicit user consent before redirecting funds to the treasury.

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!