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

Double-Counting Tokens in `_handleReturn()`Leads to Over-Withdrawal

cipherhawk

Summary:

->clear logic flaw in _handleReturn() that any savvy user can exploit to over-withdraw.

Fixing this is completely necessary to preventing share dilution and preserving fair distribution among vault participants. If you have more code or folders to share, I will continue my deep analysis and only report proven vulnerabilities.

RootCause & Where It Happens

Look at _handleReturn():

function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
(uint256 depositId) = flowData;
uint256 shares = depositInfo[depositId].shares;
uint256 amount;
​
if (positionClosed) {
// If the position is fully closed, user’s amount is (vaultBalance * theirShares / totalShares).
amount = collateralToken.balanceOf(address(this)) * shares / totalShares;
} else {
// The vault is still open after a partial GMX close or swap:
uint256 balanceBeforeWithdrawal = collateralToken.balanceOf(address(this)) - withdrawn;
amount = withdrawn + balanceBeforeWithdrawal * shares / totalShares;
}
​
// Then `_transferToken(depositId, amount);` sends `amount` to the user
...
}
​

The problem is the else branch, which adds withdrawn + (balanceBeforeWithdrawal * shares / totalShares). But withdrawn tokens are already part of the vault’s total collateral balance at the time this function runs. By adding all of withdrawn on top of the user’s fractional share of balanceBeforeWithdrawal, the user effectively double-counts the newly received tokens from GMX. This inflates their withdrawal beyond their fair share.

Concrete Example Demonstrating the Theft

  1. Initial Setup (just an example):

    • The vault has 300 total collateral tokens right before _handleReturn() is called.

    • totalShares = 100.

    • User has shares = 50 (i.e., they own half the vault).

    • The GMX callback just returned 100 new tokens into the vault. So now the vault’s total token balance on-chain is 300.

  2. How _handleReturn() is Invoked:

    • In the GMX callback, the contract sets withdrawn = 100 (the newly arrived tokens) and then calls _handleReturn(100, false, false).

  3. What the Code Does:

    • balanceBeforeWithdrawal = collateralToken.balanceOf(address(this)) - withdrawn = 300 - 100 = 200.

    • amount = withdrawn + balanceBeforeWithdrawal * shares / totalShares.

    • That is 100 + 200 * (50 / 100) = 100 + 100 = 200.

  4. Over-Withdrawal:

    • The user ends up getting 200 tokens even though they only owned 50% of the vault.

    • The vault total was 300, so the user’s correct share should have been 150.

    • They effectively steal an extra 50 tokens from everyone else in the vault.

PoC

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;
​
import "forge-std/Test.sol";
import "../src/PerpetualVault.sol"; // adjust path as needed
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
/**
* @dev Minimal mock ERC20 token used as collateral.
*/
contract MockCollateral is ERC20 {
constructor() ERC20("MockCollateral", "MCK") {
_mint(msg.sender, 1_000_000 ether); // Mint plenty for tests
}
}
​
/**
* @dev A harness version of PerpetualVault exposing internal functions and state for testing.
* It overrides _transferToken to capture the computed withdrawal amount instead of actually transferring tokens.
*/
contract HandleReturnHarness is PerpetualVault {
uint256 public lastHandledReturn;
​
// --- SETTERS for testing (these are for our harness only) ---
​
function setCollateralToken(address _collateralToken) external {
// For testing, we allow setting collateralToken
// (In production this is fixed in initialize.)
// This uses a direct assignment since collateralToken is public.
collateralToken = IERC20(_collateralToken);
}
​
function setDepositInfo(uint256 depositId, uint256 _shares, uint256 _amount) external {
depositInfo[depositId].shares = _shares;
depositInfo[depositId].amount = _amount;
// For testing, set a dummy owner (we use msg.sender)
depositInfo[depositId].owner = msg.sender;
}
​
function setCounter(uint256 _counter) external {
counter = _counter;
}
​
function setTotalShares(uint256 _totalShares) external {
totalShares = _totalShares;
}
​
// --- Override _transferToken to capture the computed withdrawal amount ---
function _transferToken(uint256, uint256 amount) internal override {
lastHandledReturn = amount;
}
​
// Expose _handleReturn as a public function for testing.
// Note: In production, _handleReturn is internal and used within a flow.
function testHandleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) external {
_handleReturn(withdrawn, positionClosed, refundFee);
}
}
​
/**
* @title DoubleCountingHandleReturnTest
* @dev This Foundry test demonstrates that _handleReturn() double-counts withdrawn tokens.
*
* Scenario:
* - Vault collateral balance before withdrawal: 300 tokens.
* - totalShares = 100.
* - A user (Alice) holds 50 shares (50% of the vault).
* - GMX callback delivers withdrawn = 100 tokens.
*
* Expected correct share: 300 * 50% = 150 tokens.
* Faulty calculation:
* balanceBeforeWithdrawal = 300 - 100 = 200,
* amount = withdrawn + (200 * 50 / 100) = 100 + 100 = 200 tokens.
* Over-withdrawal: 200 - 150 = 50 extra tokens.
*/
contract DoubleCountingHandleReturnTest is Test {
HandleReturnHarness vault;
MockCollateral collateral;
address alice = address(0x123);
​
function setUp() public {
// Deploy the mock collateral token.
collateral = new MockCollateral();
​
// Deploy the vault harness.
vault = new HandleReturnHarness();
​
// Set the vault's collateral token.
vault.setCollateralToken(address(collateral));
​
// Transfer 300 tokens to the vault (simulate existing collateral).
// Using 18 decimals: 300 ether means 300 tokens.
collateral.transfer(address(vault), 300 ether);
​
// Set up deposit info:
// - Assume depositId = 1 for Alice.
// - Alice holds 50 shares.
// - totalShares = 100.
// - (The actual deposit amount is arbitrary for share calc here.)
vm.prank(alice);
vault.setDepositInfo(1, 50, 1000 ether);
​
vault.setCounter(1);
vault.setTotalShares(100);
}
​
function testDoubleCounting() public {
// At this point:
// - Vault collateral balance is 300 tokens.
// - Alice holds 50 shares out of 100 total.
// Now, simulate a GMX callback that delivers an extra 100 tokens.
// For the purpose of _handleReturn, the parameter 'withdrawn' = 100 tokens.
//
// Expected:
// balanceBeforeWithdrawal = 300 - 100 = 200 tokens.
// Calculated amount = 100 + 200*(50/100) = 100 + 100 = 200 tokens.
// Correct share if computed as totalBalance * (shares/totalShares) should be 300*50/100 = 150 tokens.
//
// Thus, the bug causes an over-withdrawal of 50 tokens.
//
// Call our exposed function.
vault.testHandleReturn(100 ether, false, false);
​
uint256 withdrawnAmount = vault.lastHandledReturn();
emit log_named_uint("Computed withdrawal amount", withdrawnAmount);
uint256 expectedBuggyAmount = 200 ether; // As computed by the flawed logic.
uint256 correctAmount = 150 ether; // Correct proportional share.
​
assertEq(withdrawnAmount, expectedBuggyAmount, "Over-withdrawal bug not reproduced");
// Optionally, check that the bug over-awards 50 tokens:
assertEq(withdrawnAmount - correctAmount, 50 ether, "Extra tokens not awarded as expected");
}
}
​

WHY is it easy for any savvy user to exploit to over-withdraw

  • No special privileges or admin role is required. A normal user who has partial shares in an open position and triggers a withdrawal can cause _handleReturn() to double-count.

  • The over-withdrawn tokens come directly at the expense of other vault participants, violating “Depositor Share Value Preservation” and “Fair Funding Fee” invariants.

  • The attack path is clear: just perform a partial close or partial GMX swap so that withdrawn > 0, then rely on the flawed calculation.

Severity

High A withdrawing user can forcibly receive more than their fair share of the vault’s collateral. This results in direct, provable theft from remaining depositors.

FIX

  • If the vault is still open (positionClosed == false), the user should only receive their fractional share of the vault’s entire post-transaction balance, including any newly added tokens.

  • One simpler approach is to unify the logic with the “position closed” path:

    amount = collateralToken.balanceOf(address(this)) * shares / totalShares;
    ​

    That means “the user always receives a fraction of the entire vault’s current collateral, equal to their fraction of total shares.”

  • If there is some reason to treat newly arrived tokens differently, you must ensure not to double-count them. For instance, if you want to separate “immediate proceeds from GMX” vs. “existing vault balance,” you’d need correct sub-balances, not simply add them together.

Updates

Lead Judging Commences

n0kto Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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