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

Wrong Execution-Fee Refund in _handleReturn()

cipherhawk

Oversight in _handleReturn()

Use this -> Replace depositInfo[counter] with depositInfo[depositId]

Root Cause & Where It Happens

Look at the snippet from _handleReturn() (near the end of the function):

if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[depositId].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(
depositInfo[counter].owner,
depositInfo[counter].executionFee - usedFee
) {} catch {}
}
}
​

Notice the mismatch:

  1. The if-check uses:

    if (depositInfo[depositId].executionFee > usedFee) { ... }
    ​

    That’s correct for the deposit currently being withdrawn (depositId).

  2. But the actual refund call references:

    depositInfo[counter].owner
    depositInfo[counter].executionFee
    ​

    instead of depositInfo[depositId].owner and depositInfo[depositId].executionFee.

This is essentially the same category of bug exist in _cancelFlow(), but in a different location.

Proven Loss

When a user finalizes their withdrawal and triggers _handleReturn(..., refundFee = true), the code intends to refund any leftover gas fee to that user’s deposit.

  • Instead, it refunds to whatever deposit is at depositInfo[counter].

    • If counter points to a different user’s deposit (which it almost always will, especially if more deposits happened after the withdrawing user’s deposit was created), the wrong user gets the leftover fee.

    • If depositInfo[counter] is unset or already deleted, this call will effectively do nothing or revert in the try block.

Either ways, the user who rightfully should have gotten some portion of their leftover execution fee is denied that refund. This can't be theoretical: the code path is there, it will definitely misdirect or fail the refund whenever _handleReturn() tries to do a refundExecutionFee() call.

PoC

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;
​
import "forge-std/Test.sol";
import "../src/PerpetualVault.sol"; // or actual path to your PerpetualVault
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
/**
* Mock GmxProxy that does nothing but track refund calls
*/
contract MockGmxProxy {
// Track refunds
address public lastRefundRecipient;
uint256 public lastRefundAmount;
​
function refundExecutionFee(address recipient, uint256 amount) external {
lastRefundRecipient = recipient;
lastRefundAmount = amount;
}
}
​
/**
* Minimal test contract showing how leftover fee is refunded to the wrong deposit
*/
contract WrongExecutionFeeRefundTest is Test {
PerpetualVault vault;
MockGmxProxy mockProxy;
​
address alice = address(0xA1);
address bob = address(0xB1);
​
function setUp() public {
// Deploy PerpetualVault (assume no constructor needed or it’s upgradeable)
vault = new PerpetualVault();
​
// Pretend we have a GmxProxy
mockProxy = new MockGmxProxy();
​
// For demonstration, we forcibly set `vault.gmxProxy = address(mockProxy)`
// In real code, you might call something like: vault.setGmxProxy(...)
// We'll do direct storage writes or a set function if it exists
// e.g. vault.gmxProxy() = mockProxy; (pseudocode)
​
// Next: set up deposit data
// We want deposit #1 with executionFee = 1 ether, deposit #2 with executionFee=0
// Also set `counter=2`, so the vault thinks the next deposit is #2
// Then we’ll forcibly call `_handleReturn(...)`.
​
// In real usage, depositInfo, totalShares, etc. are internal,
// so for this PoC we might do direct writes with `vm.store`.
// A simpler approach is to mock the vault or set it up in a specialized way.
// Below, we do a partial approach: we craft a derived or harness contract if necessary.
​
// For brevity, let's do a harness approach:
HarnessVault hv = new HarnessVault();
hv.setGmxProxy(address(mockProxy));
// deposit #1 => executionFee=1 ether
hv.setDepositInfo(1, 1 ether, alice);
hv.setDepositInfo(2, 0, bob);
hv.setCounter(2);
​
vault = PerpetualVault(address(hv));
}
​
/**
* Prove leftover fee is refunded to depositInfo[counter] not depositInfo[depositId].
*/
function testRefundGoesToWrongDeposit() public {
// We'll simulate a call to `_handleReturn(...)`
// The actual call normally occurs after a GMX callback in a flow = WITHDRAW scenario.
// We'll directly call harness to replicate the bug.
​
// The deposit #1 is flowData, so depositId=1 is the legitimate withdrawer.
// But the vault's `counter=2`, so the leftover fee is incorrectly sent to deposit #2's owner (Bob).
​
vm.startPrank(address(vault)); // So it can call its own internal function
// If the vault is harness, we can call a public harness function to wrap `_handleReturn(...)`.
HarnessVault(address(vault)).callHandleReturn(100, true, true);
vm.stopPrank();
​
// Now check the mockProxy records for the leftover fee
address actualRecipient = mockProxy.lastRefundRecipient();
uint256 actualAmount = mockProxy.lastRefundAmount();
​
// We expect that leftover fee should have gone to `alice`, but we see if it goes to `bob`.
console.log("Expected leftover fee recipient: alice(", alice, ")");
console.log("Actual leftover fee recipient: ", actualRecipient);
​
require(actualRecipient == bob,
"Wrong leftover fee not proven: it should have gone to deposit #1's owner (alice)."
);
require(actualAmount == 1 ether,
"Did not refund the leftover fee from deposit #1's data."
);
​
// => The test fails or reverts if the code was fixed. If unpatched, it shows the mismatch.
}
}
​
​
/**
* Minimal harness exposing relevant vault fields and `_handleReturn`.
*/
contract HarnessVault is PerpetualVault {
// Let us forcibly set depositInfo
function setDepositInfo(uint256 depositId, uint256 executionFee, address ownerAddr) external {
depositInfo[depositId].executionFee = executionFee;
depositInfo[depositId].owner = ownerAddr;
depositInfo[depositId].shares = 1000; // arbitrary
depositInfo[depositId].amount = 1000; // arbitrary
}
​
function setCounter(uint256 c) external {
counter = c;
}
​
function setGmxProxy(address px) external {
gmxProxy = IGmxProxy(px);
}
​
// Expose the `_handleReturn(...)` as public for test
function callHandleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) external {
flowData = 1; // depositId=1 is the legit deposit
_handleReturn(withdrawn, positionClosed, refundFee);
}
}
​

Severity by Likelyhood & impact

This is at least medium severity: although it does not allow vault draining, it results in direct user fund loss (the leftover execution fee) whenever a withdrawal flow tries to refund. Over time, multiple users can be affected if they frequently deposit/withdraw and rely on leftover fee refunds.

FIX

In _handleReturn():

if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[depositId].executionFee > usedFee) {
// Correct references:
try IGmxProxy(gmxProxy).refundExecutionFee(
depositInfo[depositId].owner,
depositInfo[depositId].executionFee - usedFee
) {} catch {}
}
}
​

This parallels the fix you would do in _cancelFlow(), ensuring we always reference depositInfo[depositId] instead of depositInfo[counter].

Updates

Lead Judging Commences

n0kto Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_counter_invalid_during_handleReturn

Likelihood: Medium/High, when withdraw on a 1x vault. Impact: High, the fees will be distributed to the last depositor and not the withdrawer.

Support

FAQs

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