Salty.IO - 0xBinChook's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L154

Vulnerability details

Title

Tiny collateral share increase prevents liquidation

Links to affected code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L154

Vulnerability details

Impact

To maintain the invariant of USDS overcollateralization, it is imperative to permit the liquidation of positions that fall below the required collateral threshold.

When a user becomes undercollateralized due to market fluctuations, they can prevent their liquidation by using the modification cooldown, that serves as a temporary shield against liquidation, providing a window during which no liquidation actions can be taken.

A user can continually prevent liquidation by adding a minimal amount of collateral at the conclusion of each cooldown period, effectively granting an indefinite reprieve from liquidation. Transaction ordering could allow for a liquidation transaction to precede the collateral increase, but the presence of a cap on liquidation rewards introduces an economic factor, incentivizing liquidators to moderate their gas expenditures to ensure that the liquidation remains financially viable.

By default, the reward cap is only $500 USD. To order share increase before any liquidations, the gas price merely needs to push the liquidation transaction cost above that. With a constant cost in execution, the strategy becomes more optimal the larger the share (the larger the amount of USDS undercollateralized), which are the holding you need liquidated as they have the greatest impact on overall collateralization.

To push the liquidation transaction gas cost above $500 USD with the increase share gas is ~$667 USD, proportionally a trivial sum for a seriously large holding.

Proof of Concept

Steps

1. Deposit collateral
2. Mint maximum USDS
3. Wait
4. Position undercollateralized
5. Deposit tiny amount of collateral
6. Await end of cooldown period and repeat step 5.

Invariant: USDS is overcollateralized

The Salty.io documentation

USDS is the overcollateralized stablecoin native to the Salty.IO ecosystem which is pegged to the US Dollar and uses deposited WBTC/WETH LP as collateral.

Tiny increase of collateral is allowed

In the flow for CollateralAndLiquidity::depositCollateralAndIncreaseShare(), minimum values are only required by Pools::addLiquidity() and StakedRewards::increaseUserShare().

The amount of both tokens must be greater than dust, in Pools::addLiquidity()

		require( maxAmountA > PoolUtils.DUST, "The amount of tokenA to add is too small" );
		require( maxAmountB > PoolUtils.DUST, "The amount of tokenB to add is too small" );

The share must be greater than zero, in StakingRewards::_increaseUserShare()

		require( increaseShareAmount != 0, "Cannot increase zero share" );

Modification Cooldown period

Default of 1 hour, with potential range of 15 minutes to 6 hours, in adjustment of 15 minutes, in StakingConfig

	// Minimum time between increasing and decreasing user share in SharedRewards contracts.
	// Prevents reward hunting where users could frontrun reward distributions and then immediately withdraw.
	// Range: 15 minutes to 6 hours with an adjustment of 15 minutes
	uint256 public modificationCooldown = 1 hours;

Change in collateral share sets cooldown

When a user increase their share in CollateralAndLiquidity, the cooldown is updated in StakingRewards

CollateralAndLiquidity::depositCollateralAndIncreaseShare()

        // Have the user deposit the specified WBTC/WETH liquidity and increase their collateral share
    (addedAmountWBTC, addedAmountWETH, addedLiquidity) = _depositLiquidityAndIncreaseShare( wbtc, weth, maxAmountWBTC, maxAmountWETH, minLiquidityReceived, useZapping );

    emit CollateralDeposited(msg.sender, addedAmountWBTC, addedAmountWETH, addedLiquidity);

Liquidity::_depositLiquidityAndIncreaseShare()

		// Increase the user's liquidity share by the amount of addedLiquidity.
		// Cooldown is specified to prevent reward hunting (ie - quickly depositing and withdrawing large amounts of liquidity to snipe rewards as they arrive)
		// _increaseUserShare confirms the pool as whitelisted as well.
		_increaseUserShare( msg.sender, poolID, addedLiquidity, true );

CollateralAndLiquidity::_depositLiquidityAndIncreaseShare()

        // Have the user deposit the specified WBTC/WETH liquidity and increase their collateral share
    (addedAmountWBTC, addedAmountWETH, addedLiquidity) = _depositLiquidityAndIncreaseShare( wbtc, weth, maxAmountWBTC, maxAmountWETH, minLiquidityReceived, useZapping );

    emit CollateralDeposited(msg.sender, addedAmountWBTC, addedAmountWETH, addedLiquidity);

StakingRewards::_increaseUserShare()

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

Modification Cooldown prevents liquidation

To liquidate a user, their share must be decreased, but when decreasing the user's share, true is given for using the cooldown. When block.timestamp is within the modified cooldown window the transaction reverts.

CollateralAndLiquidity::liquidateUser()

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

Reward cap

Liquidation rewards are capped, initially at $500 USD.

Liquidation rewards are DAO configurable parameter in StableConfig

	// The maximum reward value (in USD) that a user receives for instigating the liquidation process.
	// Range: 100 to 1000 with an adjustment of 100 ether
  uint256 public maxRewardValueForCallingLiquidation = 500 ether;

Reward capping being performed in CollateralAndLiquidity::liquidateUser()

		// Make sure the value of the rewardAmount is not excessive
		uint256 rewardValue = underlyingTokenValueInUSD( rewardedWBTC, rewardedWETH ); // in 18 decimals
		uint256 maxRewardValue = stableConfig.maxRewardValueForCallingLiquidation(); // 18 decimals
		if ( rewardValue > maxRewardValue )
			{
			rewardedWBTC = (rewardedWBTC * maxRewardValue) / rewardValue;
			rewardedWETH = (rewardedWETH * maxRewardValue) / rewardValue;
			}

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

Gas Usage

A successful CollateralAndLiquidity::depositCollateralAndIncreaseShare() uses ~134% more gas than a successful CollateralAndLiquidity::liquidateUser().

<details> <summary>Gas Report</summary>

Running 1 test for src/stable/tests/CollateralAndLiquidity.t.sol:TestCollateral
[PASS] test_liquidation() (gas: 1111192)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.28ms
| src/stable/CollateralAndLiquidity.sol:CollateralAndLiquidity contract |                 |        |        |        |         |
|-----------------------------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost                                                       | Deployment Size |        |        |        |         |
| 4650324                                                               | 25779           |        |        |        |         |
| Function Name                                                         | min             | avg    | median | max    | # calls |
| borrowUSDS                                                            | 104722          | 123372 | 123372 | 142022 | 2       |
| depositCollateralAndIncreaseShare                                     | 127771          | 179886 | 157771 | 254118 | 3       |
| liquidateUser                                                         | 94333           | 142333 | 142333 | 190333 | 2       |
| liquidizer                                                            | 296             | 296    | 296    | 296    | 1       |
| maxBorrowableUSDS                                                     | 14680           | 27430  | 27430  | 40180  | 2       |

Add the test case to src/stable/tests/CollateralAndLiquidity.t.sol

	// Minimal liquidation test for clocking gas usage, 2 liquidations to include storage warm up in metrics
	function test_liquidation() external {
		vm.startPrank(bob);
		collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(bob), weth.balanceOf(bob), 0, block.timestamp, false );

		_depositHalfCollateralAndBorrowMax(alice);
		_crashCollateralPrice();
		vm.warp( block.timestamp + 1 days );
		collateralAndLiquidity.liquidateUser(alice);

		_depositHalfCollateralAndBorrowMax(alice);
		_crashCollateralPrice();
		vm.warp( block.timestamp + 1 days );
		collateralAndLiquidity.liquidateUser(alice);
	}

To include the gas report add this to foundry.toml

gas_reports = ["CollateralAndLiquidity"]

When running the test add --gas-report to the command to generate the gas usage table.

</details>

Test Case

<details> <summary>Test Case</summary> Demonstrates the increase of collateral share by a tiny amount prevents liquidation.

Add the test case to src/stable/tests/CollateralAndLiquidity.t.sol

	function test_preventing_liquidation_using_cooldown() external {
    // Tiny deposit must still be larger than DUST after zapping
    uint256 tinyDeposit = 2*PoolUtils.DUST;

    // Bob deposits collateral to keep pool reserves above DUST after Alice's liquidation
    vm.startPrank(bob);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(bob), weth.balanceOf(bob), 0, block.timestamp, false );

    // Alice will deposit collateral and mints maximum allowed USDS
    _depositHalfCollateralAndBorrowMax(alice);

    // Artificially crash the collateral price
    _crashCollateralPrice();

    // Delay before the liquidation
    vm.warp( block.timestamp + 1 days );

    // Alice changes the cooldown semaphore by increases their share
    vm.prank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(tinyDeposit, tinyDeposit, 0, block.timestamp, true );

    // Alice can still be liquidated
    assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice), "Alice can be liquidated");

    // Liquidation is prevent due to cooldown
    vm.expectRevert("Must wait for the cooldown to expire");
    collateralAndLiquidity.liquidateUser(alice);
}

</details>

Tools Used

Manual review, Foundry test

Recommended Mitigation Steps

Allow liquidation irrespective of the user's cooldown status.

In CollateralAndLiquidity ::liquidateUser()

		// 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 );
		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );

		// The caller receives a default 5% of the value of the liquidated collateral.
		uint256 rewardPercent = stableConfig.rewardPercentForCallingLiquidation();

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L320-L320 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L385-L400 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L281-L281

Vulnerability details

Impact

Once a user casts their vote on a proposal, it may have reached the required quorum, if not, the user can unstake their xSalt, thereby lowering the quorum threshold needed. This action doesn't affect the validity of their vote, as it remains counted even after they have unstaked.

Users have the flexibility to cancel their unstaking before the end of the period, without incurring any penalties. They receive back their entire initial stake. The only cost involved in this process is the execution gas fee.

This system presents an opportunity for significant manipulation. A user with a large holding, or a group of users working in coordination, could exploit this mechanism. They could strategically unstake to influence the quorum, enabling the passage of proposals that might not have succeeded under normal circumstances.

Proof of Concept

Steps

1. Vote on a proposal
2. Proposal quorum not met, cannot be finalized
3. Unstake xSalt
4. Reduced the required quorum

If the shortfall in quorum was closed by the reduction (either very close, or a larger holder) then the proposal can now be finalized. Alternatively await collaborators to also execute steps to finalized proposal, then everyone can cancel their unstakes.

To finalize the ballot you need the required quorum

When the DAO decides whether a proposal can be finalized it uses Proposals::canFinalizeBallot()

	function canFinalizeBallot( uint256 ballotID ) external view returns (bool)
		{
        Ballot memory ballot = ballots[ballotID];
        if ( ! ballot.ballotIsLive )
        	return false;

        // Check that the minimum duration has passed
        if (block.timestamp < ballot.ballotMinimumEndTime )
            return false;

        // Check that the required quorum has been reached
        if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
            return false;

        return true;
	    }

The number of cast votes must be greater than or equal to the required quroum requiredQuorumForBallotType( ballot.ballotType )

Required quorum is dynamic

The quorum threshold is calculated from the staked Salt amount on each call to Proposals::requiredQuorumForBallotType()

	function requiredQuorumForBallotType( BallotType ballotType ) public view returns (uint256 requiredQuorum)
		{
		// The quorum will be specified as a percentage of the total amount of SALT staked
		uint256 totalStaked = staking.totalShares( PoolUtils.STAKED_SALT );
		require( totalStaked != 0, "SALT staked cannot be zero to determine quorum" );

		if ( ballotType == BallotType.PARAMETER )
			requiredQuorum = ( 1 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
		else if ( ( ballotType == BallotType.WHITELIST_TOKEN ) || ( ballotType == BallotType.UNWHITELIST_TOKEN ) )
			requiredQuorum = ( 2 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
		else
			// All other ballot types require 3x multiple of the baseQuorum
			requiredQuorum = ( 3 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );

		// Make sure that the requiredQuorum is at least 0.50% of the total SALT supply.
		// Circulating supply after the first 45 days of emissions will be about 3 million - so this would require about 16% of the circulating
		// SALT to be staked and voting to pass a proposal (including whitelisting) 45 days after deployment..
		uint256 totalSupply = ERC20(address(exchangeConfig.salt())).totalSupply();
		uint256 minimumQuorum = totalSupply * 5 / 1000;

		if ( requiredQuorum < minimumQuorum )
			requiredQuorum = minimumQuorum;
		}

The totalStaked is used as base for the three types of ballotType and will be at least be minimumQuorum

Importantly before the unstaking the required quorum calculation must be above the minimum quorum, otherwise there would be no change (as both states would be the minimum quorum).

Unstaking does not reduce cast votes

The comment sums the situation clearly in Proposals::castVote()

		// If the user changes their stake after voting they will have to recast their vote.

If a user unstakes their full holding, they are unable to recast their vote due to checks in Proposals::castVote()

		uint256 userVotingPower = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
		require( userVotingPower > 0, "Staked SALT required to vote" );

Test Case

<details> <summary>Test Case</summary> Demonstrates unstaking reducing the required quorum, with no effect on the total votes cast by unstaking.

Add the test case to Proposals.t.sol

    function test_manipulate_quorum_by_unstaking() external {
    uint256 ballotID = 1;
    uint256 stakeAmount = 2500000 ether;

    // Fund each with equal SALT for staking
    vm.startPrank( DEPLOYER );
    salt.transfer( alice, stakeAmount );
    salt.transfer( bob, stakeAmount );
    vm.stopPrank();

    // The total amount of staked salt will be above the minimum requirement
    _stakeSalt(alice, stakeAmount);
    _stakeSalt(bob, stakeAmount);

    // Alice proposes a ballot
    vm.prank(alice);
    proposals.proposeCountryInclusion("US", "proposed ballot");

    // Alice votes YES
    vm.prank(alice);
    proposals.castVote( ballotID, Vote.YES );

    uint256 quorumBeforeUnstake = proposals.requiredQuorumForBallotType(BallotType.INCLUDE_COUNTRY);
    uint256 totalVotesBeforeUnstake = proposals.totalVotesCastForBallot(ballotID);

    // Alice unstake xSalt
    vm.prank(alice);
    staking.unstake(stakeAmount, 52);

    uint256 quorumAfterUnstake = proposals.requiredQuorumForBallotType(BallotType.INCLUDE_COUNTRY);
    uint256 totalVotesAfterUnstake = proposals.totalVotesCastForBallot(ballotID);

    // Votes are unchanged by unstaking, while the required quorum is reduced
    assertEq(totalVotesBeforeUnstake, totalVotesAfterUnstake, "Total votes unchanged by staking");
    assertTrue(quorumBeforeUnstake != quorumAfterUnstake, "Require quorum has changed by unstaking");
    assertGt(quorumBeforeUnstake, quorumAfterUnstake, "Quorum is greater than before unstaking");
}

    function _stakeSalt(address wallet, uint256 amount ) private {
        vm.startPrank( wallet );
        salt.approve(address(staking), amount);
        staking.stakeSALT(amount);
        vm.stopPrank();
    }

</details>

Tools Used

Manual review, Foundry test

Recommended Mitigation Steps

When a user unstakes (partial or fully), unless they recast their vote (with their reduced balance), the votes remain when the stake does not.

The ideal would be to have the users votes cast altered when their stake is reduced, however that's unlikely to be practical, due to there being no limit on the number of potential proposals they can have voted on, or the total number of available proposals.

An easy option is to set the required quorum for a proposal when it is created (using the xSalt amount then). Alternatively a combination of the current approach using the required quorum when the proposal was created as an additional minimum, would account for an expanding amount of staked Salt.

Add the member minimumRequiredQuorum to IProposals::Ballot

	string description;

+   // The required quorum when the ballot was created
+   uint256 minimumRequiredQuorum;

	// The earliest timestamp at which a ballot can end. Can be open longer if the quorum has not yet been reached for instance.
	uint256 ballotMinimumEndTime;
	}

Populate minimumRequiredQuorum in Proposoals::_possiblyCreateProposal()

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

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

Change the function signature and minimumRequiredQuorum as an additional minimum quorum bar in Proposals::requiredQuorumForBallotType()

-   function requiredQuorumForBallotType( BallotType ballotType ) public view returns (uint256 requiredQuorum)
+   function requiredQuorumForBallotType( BallotType ballotType, uint256 minimumRequiredQuorum ) public view returns (uint256 requiredQuorum)
		{
		// The quorum will be specified as a percentage of the total amount of SALT staked
		uint256 totalStaked = staking.totalShares( PoolUtils.STAKED_SALT );
		require( totalStaked != 0, "SALT staked cannot be zero to determine quorum" );

		if ( ballotType == BallotType.PARAMETER )
			requiredQuorum = ( 1 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
		else if ( ( ballotType == BallotType.WHITELIST_TOKEN ) || ( ballotType == BallotType.UNWHITELIST_TOKEN ) )
			requiredQuorum = ( 2 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
		else
			// All other ballot types require 3x multiple of the baseQuorum
			requiredQuorum = ( 3 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );

		// Make sure that the requiredQuorum is at least 0.50% of the total SALT supply.
		// Circulating supply after the first 45 days of emissions will be about 3 million - so this would require about 16% of the circulating
		// SALT to be staked and voting to pass a proposal (including whitelisting) 45 days after deployment..
		uint256 totalSupply = ERC20(address(exchangeConfig.salt())).totalSupply();
		uint256 minimumQuorum = totalSupply * 5 / 1000;

		if ( requiredQuorum < minimumQuorum )
			requiredQuorum = minimumQuorum;
+
+       if( requiredQuorum < minimumRequiredQuorum )
+           requiredQuorum = minimumRequiredQuorum;            
		}

Elsewhere, zero can be passed for minimumRequiredQuorum to Proposals::requiredQuorumForBallotType() to keep existing behaviour.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L396-L397 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L281-L281

Vulnerability details

Impact

A staker is restricted to sponsoring a single proposal at any given time. This proposal remains active until a predetermined duration has elapsed, and it has received the sufficient number of votes to reach a quorum, after which it can be finalized. However, if a staker sponsors a proposal that fails to attract the necessary quorum of votes, it remains indefinitely in an active state. This situation effectively locks the staker in a position where they cannot propose any new initiatives, as they are stuck with an unresolved proposal.

There is no mechanism for sponsors to withdraw or cancel their proposals. So when a proposal is unable to achieve quorum, the sponsor is left in a predicament where they are unable to further participate in the governance through the initiation of new proposals.

Furthermore, there is a noticeable lack of motivation for stakers to vote against proposals that are not on track to meet the quorum. As these proposals cannot pass without achieving the required quorum, resulting in a situation where voting against such proposals does not offer any tangible benefit.

Proof of Concept

Steps

1. Staker creates a valid proposal
2. Sufficient time passes
3. Insufficient votes are received to reach quorum
4. Staker has an active proposal they cannot close (preventing create a new proposal)

A staker can have only one active proposal

Within Proposals stakers are identified as users and when creating a proposal there is a check Proposals::_possiblyCreateProposal()

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

With the state being pushed later in Proposals::_possiblyCreateProposal()

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

A proposal can only be finalized after sufficient time has passed

The check to ensure the minimum time has passed in Proposals::canFinalizeBallot()

        // Check that the minimum duration has passed
        if (block.timestamp < ballot.ballotMinimumEndTime )
            return false;

A proposal can only be finalized after quorum is reached

The check to ensure quorum is reached in Proposals::canFinalizeBallot()

        // Check that the required quorum has been reached
        if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
            return false;

<details> <summary>Test Case</summary> User sponsors a proposal that fails to reach quorum cannot be finalized and with no other way to close will have an active proposal, preventing further proposals.

Add the test case to Proposals.t.sol

    function test_quorum_needed_to_finalize_ballot() external {
        uint256 ballotID = 1;
        uint256 stakeAmount = 250000 ether;

        // Fund Alice, Bob and Charlie with equal SALT
        vm.startPrank( DEPLOYER );
        salt.transfer( alice, stakeAmount );
        salt.transfer( bob, stakeAmount );
        salt.transfer( charlie, stakeAmount );
        vm.stopPrank();

        // Alice, Bob and Charlie all stake their equal amounts of SALT
        _stakeSalt(alice, stakeAmount);
        _stakeSalt(bob, stakeAmount);
        _stakeSalt(charlie, stakeAmount);

        // Alice proposes a ballot
        vm.prank(alice);
        proposals.proposeCountryInclusion("US", "proposed ballot");

        // Alice votes YES
        vm.prank(alice);
        proposals.castVote( ballotID, Vote.YES );

        // Now, we allow some time to pass in order to finalize the ballot
        vm.warp(block.timestamp + daoConfig.ballotMinimumDuration());

        // The ballot cannot be finalized as quorum is not reached
        assertFalse(proposals.canFinalizeBallot(ballotID), "Ballot cannot be finalized");
        assertGt(proposals.requiredQuorumForBallotType(BallotType.INCLUDE_COUNTRY),stakeAmount, "Quorum exceeds stakeAmount" );
        assertTrue( proposals.userHasActiveProposal(alice), "Alice has an active proposal");
    }

    function _stakeSalt(address wallet, uint256 amount ) private {
        vm.startPrank( wallet );
        salt.approve(address(staking), amount);
        staking.stakeSALT(amount);
        vm.stopPrank();
    }

</details>

Tools Used

Manual review, Foundry test

Recommended Mitigation Steps

Allow the proposals to be closed (equivalent to finalized as NO or NO_CHANGE), which would allow the sponsor to afterward make a different proposal.

(This feature would also generally allow removing dead proposals)

Add a time field to Ballot in IProposals

	// The earliest timestamp at which a ballot can end. Can be open longer if the quorum has not yet been reached for instance.
	uint256 ballotMinimumEndTime;

+	// The earliest timestamp at which a ballot can be closed without quorum being reached.
+	uint256 ballotCloseTime;
	}

Populate the ballotCloseTime in Proposal::_possiblyCreateProposal , using a constant in this example, it could always another DAO configuration option.

		// 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();
+       uint256 ballotCloseTime = ballotMinimumEndTime + 1 weeks;		

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

Add a function to return whether a proposal can be closed to Proposal

+	function canCloseBallot( uint256 ballotID ) external view returns (bool)
+		{
+        Ballot memory ballot = ballots[ballotID];
+        if ( ! ballot.ballotIsLive )
+        	return false;
+
+        // Check that the minimum duration has passed
+        if (block.timestamp < ballot.ballotCloseTime )
+            return false;
+
+        return true;
+	    }

Add a function to close a ballot without any side effect to DAO

+   function closeBallot( uint256 ballotID ) external nonReentrant
+       {
+       // Checks that ballot is live and closeTime has passed
+		require( proposals.canCloseBallot(ballotID), "The ballot is not yet able to be closed" ); 
+
+       // No mutation from the propsal
+		_finalizeApprovalBallot(ballotID);
+       }

Assessed type

Governance

QA Report for Salty.io

Low Risk

L-1

Staking configuration changes only applies to new unstake after finalization

to the StakingConfig alter the conditions for users wishing to unstake. Users who are already unstaking at the time of these changes will not be affected by them if the changes are advantageous, such as a decrease in the time required to unstake their complete share.

Recommended Mitigation

Include the maxUnstakeWeeks in the Unstake struct and provide a function for users to call. to reduce their staking time when the criteria have been altered with governance.

L-2

Whitelisting, un-whitelisting then re-whitelisting to game bootstrap rewards

In the DAO governance process, when a token is approved through a proposal to be whitelisted, it becomes eligible for the bootstrappingRewards in SALT. There are no safeguards on the bootstrappingRewards for the same token being whitelisted again after it has been un-whitelisted, allowing the exploited of repeatedly listing and un-whitelisting tokens in order to artificially inflate the rewards received by the pool.

Recommended Mitigation

Keep a mapping of un-whitelisted tokens, and only provide bootstrappingRewards to no in that mapping.

L-3

Price feed proposal are not check to ensure feed uniqueness

There is no verification to confirm that when the price feed from a proposal is applied, the three price feed addresses are distinct. If two or more of these feeds were directed to the same address, it would undermine the purpose of having a redundant feed system. As the two identical feeds would produce agreeing results, eliminating the benefit of having multiple sources for price information.

Recommended Mitigation

In DAO::_executeSetContract(), before applying the mutation check the three price feeds would remain unique.

L-4

Rounding down prevents proposals when there is between 1 and 999 xSALT

With the lowest required proposal stake configuration, if the sum of staked SALT is between 1 and 999 xSALT then no proposals can be made, even though the proposer may have xSALT.

In Proposals::_possiblyCreateProposal()

    uint256 requiredXSalt = ( totalStaked * daoConfig.requiredProposalPercentStakeTimes1000() ) / ( 100 * 1000 );

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

requiredXSalt is zero, due to the denominator being larger than the numerator.

Recommended Mitigation

If expected behaviour then document in NatSpec, otherwise use a default minimum value for requiredXSalt in a similar approach to required quorum.

L-5

When xSalt is low, there is a risk of proposal spam

The threshold for requiredXSalt is calculated using the current amount of xSalt (staked amount of SALT).

In Proposals::_possiblyCreateProposal()

    uint256 totalStaked = staking.totalShares(PoolUtils.STAKED_SALT);
    uint256 requiredXSalt = ( totalStaked * daoConfig.requiredProposalPercentStakeTimes1000() ) / ( 100 * 1000 );

When xSalt is low, the requiredXSalt will be even lower (daoConfig.requiredProposalPercentStakeTimes1000() ranges from 0.1% to 2%), with a lower bar cost to create a spam proposal (or self-interested one) more will get created.

Recommended Mitigation

Implement a default minimum value for requiredXSalt in a similar approach to required quorum, where if requiredXSalt is below the minimum use that instead.

L-6

SigningTools::_verifySignature allows malleable (non-unique) signature

The signature verification used during the bootstrapBallot and by AccessManager::grantAccess() allows malleable (non-unique) signatures, as defined by EIP-2.

Recommended Mitigation

Use Open Zeppelin's ECDSA::tryRecover() or replicate the enforcement of signatures being in the lower order.

L-7

Salt from unclaimed airdrops will be trapped

The first on-chain step for the Salt airdrop, the users must cast their vote with BootstrapBallot::vote(), which registers them with Airdrop::authorizeWallet(). Following a successful bootstrap ballot that initiates the exchange, the second step has users calling Airdrop::claimAirdrop(). Failing to do so results in their allocated Salt remaining inaccessible within Airdrop.

The trapped Salt will be out of circulation, but still counted as part of the total, artificially inflating the total circulating supply.

Recommended Mitigation

Have Airdop implement ICalledContract with the callFromDAO function transferring the outstanding Salt to be burnt, effectively closing the claim window using a DAO Proposal.

L-8

ManagedWallet is an trap ETH

Receiving ETH is used as a decision signal by the confirmationWallet whenever a wallet change is proposed by the mainWallet.

ManagedWallet::receive()

    // Confirm if .05 or more ether is sent and otherwise reject.
    // Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
    if ( msg.value >= .05 ether )
        activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
    else
        activeTimelock = type(uint256).max; // effectively never

Affirmation requires 0.05 ether or greater to be transferred to ManagedWallet, but there is no mechanism for withdrawing the accused ETH, trapping it in the ManagedWallet.

Recommended Mitigation

Add a withdraw() that allows either the mainWallet or confirmationWallet to pull any ETH from the ManagedWallet.

L-9

Control of proposedConfirmationWallet should be confirmed

ManagedWallet::changeWallets() allows the proposedMainWallet to be promoted to the mainWallet and the proposedConfirmationWallet is also promoted to the cnfirmationWallet, but there has been no transaction from the proposedConfirmationWallet verifying it is controlled / correct.

If the proposedConfirmationWallet is not correct the primary function of ManagedWallet would be broken (being able to change the mainWallet).

Recommended Mitigation

Use a two-step approach, where after the proposedMainWallet confirms acceptance, the proposedConfirmationWallet may then also confirm, at which point the proposed wallets are promoted.

L-10

Both proposed wallets can be the same

ManagedWallet::proposeWallets() does not check that the two wallets are unique, allowing the same wallet to be both the main and confirmation. Having one wallet fill both roles, would defeat the purpose of using ManagedWallet, to provide security by separating the roles.

Recommended Mitigation

Add a check that the proposed wallets do not equal each other.

L-11

Upkeep has a race condition that creates an economic disincentive

Upkeep.performUpkeep is an essential part of the protocol that operates on a decentralized economic incentive model, encouraging users to execute it by offering rewards. The reward relies on a combination of swap activity and time elapsed since the last upkeep was performed.

On the Ethereum mainnet, the order of transactions within a block is generally determined by the combination of gas price and miner tip. It's possible for multiple transactions to call the same function within the same block.

The design of Upkeep.performUpkeep allows it to be called multiple times within the same block. However, after the initial call in a block, subsequent calls won't receive any reward because there's no time gap since the previous upkeep.

This setup creates a situation where users might pay gas fees to fully process a transaction without receiving any reward if they don't win the transaction ordering race. This results in an inflated penalty for users who end up losing this contest.

Recommended Mitigation

In Upkeep::performUpkeep()

	function performUpkeep() public nonReentrant
	{
+       require(lastUpkeepTimeEmissions != block.timestamp, "No time since elapsed since last upkeep"); 		
+
        // Perform the multiple steps of performUpkeep()
        try this.step1() {}
        catch (bytes memory error) { emit UpkeepError("Step 1", error); }		

L-12 The first staker gets all preloaded rewards

A newly whitelisted token receives bootstrap rewards from the reward emitter as part of the upkeep cycle.

When rewards are emitted to a LP before any providers have staked, the first provider will gain all those initial rewards, while subsequent stakers in the same upkeep cycle receive none.

Proof of concept

This is due to StakingRewards::_increaseUserShare()

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

When there are no existingTotalShares, then there are no virtualRewardsToAdd, which is the rewards accumulation counter.

<details> <summary>Test Case</summary> As the same function is shared between liquidity and staking, `poolIDs[0]` is used for brevity of example.

Add the test to src/staking/tests/StakingRewards.t.sol

    function testFirstStakerGetsReward() external {
        uint256 shareAlice = 5 ether;
        uint256 shareBob = 5 ether;
        uint256 rewards = 2 ether;

        // Assert clean start state
        assertEq(stakingRewards.userShareForPool(alice, poolIDs[0]), 0, "Alice's share should be zero");
        assertEq(stakingRewards.userShareForPool(bob, poolIDs[0]), 0, "Bob's share should be zero");
        assertEq(stakingRewards.totalShares(poolIDs[0]), 0, "Total shares should be zero");
        assertEq(stakingRewards.totalRewards(poolIDs[0]), 0, "Total rewards should be zero");
        assertEq(salt.balanceOf(address (stakingRewards)), 0, "Salt balance should be zero");

        // Add reward to the pools - before any users stake
        AddedReward[] memory addedRewards = new AddedReward[](1);
        addedRewards[0] = AddedReward(poolIDs[0], rewards);
        stakingRewards.addSALTRewards(addedRewards);

        assertEq(stakingRewards.totalRewards(poolIDs[0]), rewards, "Total rewards should be 2 ether");

        // Increase Alice's stake
        vm.startPrank(DEPLOYER);
        stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], shareAlice, true);
        vm.stopPrank();

        assertEq(stakingRewards.userShareForPool(alice, poolIDs[0]), shareAlice, "Alice's share should have increased");
        assertEq(stakingRewards.totalShares(poolIDs[0]), shareAlice, "Total rewards should be sum of Alice's share");

        // @audit Alice has earned rewards despite staking AFTER the rewards were added to the pool
        assertEq(stakingRewards.userRewardForPool(alice, poolIDs[0]), rewards, "Alice has all the rewards despite staking AFTER they were added, not before");

        // Increase Bob's stake
        vm.startPrank(DEPLOYER);
        stakingRewards.externalIncreaseUserShare(bob, poolIDs[0], shareBob, true);
        vm.stopPrank();

        assertEq(stakingRewards.userShareForPool(bob, poolIDs[0]), shareBob, "Bob's share should have increased");
        assertEq(stakingRewards.totalShares(poolIDs[0]), shareAlice + shareBob, "Total rewards should be sum of Alice's and Bob's shares");
        assertEq(stakingRewards.userRewardForPool(bob, poolIDs[0]), 0, "Bob's share is zero");
        assertEq(stakingRewards.userRewardForPool(alice, poolIDs[0]), rewards, "Alice still has the rewards, even after totalShares increase!");
    }

</details>

Recommended Mitigation

Don't emit rewards until there is at least one staker providing LP.

Non-Critical

NC-1

When a user recasts their vote emit an event

When Propsoals::castVote() is used to recast a vote no event is emitted when the prior vote is removed, but there is one for newly cast vote.

NC-2

Proposals::userHasActiveProposal() will be incorrect for the DAO

Proposals::userHasActiveProposal() determines if a given address has an active proposal. It operates under the assumption that only one open proposal per user is permitted, but the DAO is an exception and can have multiple open proposals, where this function may return false if queried after the closure of one proposal while others remain open. This is due to the function being set to return false upon the finalization of a proposal, not accounting for the possibility that the DAO could still have other ongoing proposals.

NC-3

Proposals::openBallots() returns an unbounded set

No limit is imposed on the number of open proposals. Proposals::openBallots() copies the entire set of proposals to the memory return buffer, which can cause gas issues for any calling contract, but also data or timeout limits for JSON-RPC retrieval, due to overwhelming data volume.

NC-4

Inconsistent quorum tiers between minimumQuorum and requiredQuorum

In Proposals::requiredQuorumForBallotType() when calculated tiered required quorum is below 5% of the total SALT, that is used, instead of a tiered required quorum amount.

There are three tiers for proposals, the lowest tier being for PARAMETER changes, the middle tier for WHITELIST_TOKEN and UNWHITELIST_TOKEN, and the highest tier for all other types. The quorum for the lowest tier is the baseBallot amount, for the middle tier: 2 * baseBallot, and for the top tier: 3 * baseBallot, displaying the principle that more significant the proposal the larger quorum required.

However, there's an issue with the minimumQuorum, which is a flat rate applied uniformly. When the quorums for all three tiers fall below this minimumQuorum, they are set to the same level, disregarding the different impact levels these tiers are supposed to represent in the protocol.

NC-5

Integer rounding may leave dust in Airdrop

Airdrop calculates the saltAmountForEachUser based on the number of airdrop recipients.

Airdrop::allowClaiming()

    saltAmountForEachUser = saltBalance / numberAuthorized();

Unless saltBalance % numberAuthorized() == 0, Salt dust will remain after a complete distribution to all claimants.

NC-6

Hardcoded ETH amount for confirmation signal is brittle

ManagedWallet::receieve() requires the confirmationWallet to send at least 0.05 ether to signal their agreement to the proposed wallet changes.

ManagedWallet::receive()

    // Confirm if .05 or more ether is sent and otherwise reject.
    // Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
    if ( msg.value >= .05 ether )
        activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
    else
        activeTimelock = type(uint256).max; // effectively never

While 0.05 ether is only $125 USD at an Ether price of $2,500, if the dizzying heights of $10K+ are achieved the cost will eventually be non-trivial.

NC-7

Governance update ro geo-restriction depends on off-chain private party

Liquidty::_depositLiquidityAndIncreaseShare() requires that users must have current exchange access deposit collateral. This requirement applies even if the user is trying to add collateral to their existing position, to increase their collateral health.

The ability to invalidate the exchange access of all users can be executed through a governance proposal. However, restoring access is contingent on an off-chain entity providing a signature (SigningTools.EXPECTED_SIGNER) to the user, who must then submit it to AccessManager::grantAccess().

NC-8

Staking::unstakesForUser() enumerates over an unbounded set

A user can increase the size of their set of active unstake by simply performing unstake and no limit is imposed. Staking::unstakesForUser() enumerates every one of the user's active unstake actions, which can cause gas issues for any calling contract, but also data or timeout limits for JSON-RPC retrieval, due to overwhelming data volume.

NC-9

CollateralAndLiquidty::findLiquidatableUsers() enumerates over an unbounded set

CollateralAndLiquidty::findLiquidatableUsers() enumerates over every holder borrower USDS to determine whether they are liquidate.

As there is no limit on the number of USDS borrowers, gas issues can be caused for any calling contract, but also data or timeout limits for JSON-RPC retrieval, due to overwhelming data volume.

NC-10

Exchange can be started with only a single yes vote

BootstapBallot::finalizeBallot() does not have a required quorum, it only checks whether startExchangeYes > startExchangeNo, as a result the exchange would be started if there is a single yes vote, without any no votes.