Salty.IO - haxatron's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L98

Vulnerability details

Impact

Users can only have 1 active proposal at a time

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L212C1-L219C4

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

The problem with this is that the active proposal is only cleared when markBallotAsFinalized is called

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L130-L153

	function markBallotAsFinalized( uint256 ballotID ) external nonReentrant
		{
                require( msg.sender == address(exchangeConfig.dao()), "Only the DAO can mark a ballot as finalized" );
		...
		_userHasActiveProposal[userThatPostedBallot] = false;
		}

markBallotAsFinalized can only be called from the DAO as part of when finalizing and executing a proposal. However, if the execution of the proposal fails, then a user cannot remove an active proposal.

An example of this is callFromDAO proposal which allows the DAO to call the callFromDAO function on any contract the proposer wants.

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L178-L183

		else if ( ballot.ballotType == BallotType.CALL_CONTRACT )
			{
			ICalledContract(ballot.address1).callFromDAO( ballot.number1 );

			emit ContractCalled(ballot.address1, ballot.number1);
			}

If callFromDAO in the ballot.address1 contract always reverts (it could be a mistake in the ballot.address1 contract or a malicious proxy / metamorphic contract has changed their implementation), then the proposal finalization as a whole will revert.

There will be no way for the user to clear this active proposal and thus they cannot create anymore proposals.

Tools Used

Manual Review

Recommended Mitigation Steps

Give the proposer a way to cancel their active proposal.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L317

Vulnerability details

The quorum is computed based on the totalStaked at the period when requiredQuorumForBallotType it is called

Proposals.sol#L317C1-L339C4

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

And it is called during vote finalization

Proposals.sol#L385C1-L400C7

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

However, when a user changes their stake after voting, their voting power does not change until they call castVote again

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L272C1-L277C66

	function castVote( uint256 ballotID, Vote vote ) external nonReentrant
                ...
		// Make sure that the user has voting power before proceeding.
		// Voting power is equal to their userShare of STAKED_SALT.
		// If the user changes their stake after voting they will have to recast their vote.

		uint256 userVotingPower = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
		...

Therefore a user with large share of staked SALT can first vote a proposal that does not hit the quorum and then unstake their SALT to decrease the totalStaked before quorum computation when votes are being finalised. This leads to the quorum to decrease due to the decrease in totalStaked whilst the user's voting power remains the same leading to a potential bypass of the quorum.

Tools Used

Manual Review

Recommended Mitigation Steps

Use a snapshot of the voting power and the quorum at the block time when a proposal is created.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L131 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L196-L210

Vulnerability details

Impact

When a proposal is created by any users, it will call _possiblyCreateProposal that checks:

1. whether there already is an open proposal with the same name

2. whether there already is an open proposal with the same name + "_confirm". [not important]

and reverts if so.

Proposals.sol#L81C1-L128C1

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


	// Create a confirmation proposal from the DAO
	function createConfirmationProposal( string calldata ballotName, BallotType ballotType, address address1, string calldata string1, string calldata description ) external returns (uint256 ballotID)
		{
		require( msg.sender == address(exchangeConfig.dao()), "Only the DAO can create a confirmation proposal" );

=>		return _possiblyCreateProposal( ballotName, ballotType, address1, 0, string1, description );
		}

For two proposals, the ballot names are unique for a proposal. In particular,

The sendSalt proposal always has a ballot name of sendSalt. Therefore, there can only be one active sendSalt proposal at the same time.

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L196-L210

	function proposeSendSALT( address wallet, uint256 amount, string calldata description ) external nonReentrant returns (uint256 ballotID)
		{
		require( wallet != address(0), "Cannot send SALT to address(0)" );

		// Limit to 5% of current balance
		uint256 balance = exchangeConfig.salt().balanceOf( address(exchangeConfig.dao()) );
		uint256 maxSendable = balance * 5 / 100;
		require( amount <= maxSendable, "Cannot send more than 5% of the DAO SALT balance" );

		// This ballotName is not unique for the receiving wallet and enforces the restriction of one sendSALT ballot at a time.
		// If more receivers are necessary at once, a splitter can be used.
		string memory ballotName = "sendSALT";
		return _possiblyCreateProposal( ballotName, BallotType.SEND_SALT, wallet, amount, "", description );
		}

For setContract proposals, though users can pass in a name, the name can only be one of 4 different possible names. Therefore, a user can open a proposal to setContract:priceFeed1 to prevent other users from creating proposals to change price feed 1.

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L131C1-L145C4

	function _executeSetContract( Ballot memory ballot ) internal
		{
		bytes32 nameHash = keccak256(bytes( ballot.ballotName ) );

		if ( nameHash == keccak256(bytes( "setContract:priceFeed1_confirm" )) )
			priceAggregator.setPriceFeed( 1, IPriceFeed(ballot.address1) );
		else if ( nameHash == keccak256(bytes( "setContract:priceFeed2_confirm" )) )
			priceAggregator.setPriceFeed( 2, IPriceFeed(ballot.address1) );
		else if ( nameHash == keccak256(bytes( "setContract:priceFeed3_confirm" )) )
			priceAggregator.setPriceFeed( 3, IPriceFeed(ballot.address1) );
		else if ( nameHash == keccak256(bytes( "setContract:accessManager_confirm" )) )
			exchangeConfig.setAccessManager( IAccessManager(ballot.address1) );

		emit SetContract(ballot.ballotName, ballot.address1);
		}

Tools Used

Manual Review

Recommended Mitigation Steps

For sendSalt proposals additionally include address and amount to send.

For setContract proposals additionally include the address. Note that the hash comparison system in _executeSetContract has to change as well

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L81-L128

Vulnerability details

For proposals such as SET_CONTRACT or SET_WEBSITE_URL, the DAO creates a second confirmation proposal for users to vote on after the initial proposal passes.

DAO.sol#L202

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

When a confirmation proposal is created, the ballot name of the confirmation proposal will be set to the original ballot name + _confirm.

The DAO will proceed to then create a confirmation proposal using createConfirmationProposal which calls _possiblyCreateProposal

Proposals.sol#L81C1-L128C1

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


	// Create a confirmation proposal from the DAO
	function createConfirmationProposal( string calldata ballotName, BallotType ballotType, address address1, string calldata string1, string calldata description ) external returns (uint256 ballotID)
		{
		require( msg.sender == address(exchangeConfig.dao()), "Only the DAO can create a confirmation proposal" );

=>		return _possiblyCreateProposal( ballotName, ballotType, address1, 0, string1, description );
		}

The problem in _possiblyCreateProposal is that it checks:

1. whether there already is an open proposal with the same name

2. whether there already is an open proposal with the same name + "_confirm". [not important]

and reverts if so.

However, the behaviour in 1 can be used against a protocol by creating a new SET_CONTRACT or SET_WEBSITE_URL proposal with a ballot name of an active SET_CONTRACT or SET_WEBSITE_URL proposal with the _confirm suffix appended, since creating these proposals do not check for the _confirm suffix.

Proposals.sol#L240C1-L255C4

	function proposeSetContractAddress( string calldata contractName, address newAddress, string calldata description ) external nonReentrant returns (uint256 ballotID)
		{
		require( newAddress != address(0), "Proposed address cannot be address(0)" );

		string memory ballotName = string.concat("setContract:", contractName );
		return _possiblyCreateProposal( ballotName, BallotType.SET_CONTRACT, newAddress, 0, "", description );
		}


	function proposeWebsiteUpdate( string calldata newWebsiteURL, string calldata description ) external nonReentrant returns (uint256 ballotID)
		{
		require( keccak256(abi.encodePacked(newWebsiteURL)) != keccak256(abi.encodePacked("")), "newWebsiteURL cannot be empty" );

		string memory ballotName = string.concat("setURL:", newWebsiteURL );
		return _possiblyCreateProposal( ballotName, BallotType.SET_WEBSITE_URL, address(0), 0, newWebsiteURL, description );
		}

If the initial SET_CONTRACT or SET_WEBSITE_URL proposal passes, the DAO cannot proceed with the confirmation proposal because an attacker that is unhappy with the initial SET_CONTRACT or SET_WEBSITE_URL proposal has already created a proposal with a name colliding with the confirmation proposal to be set.

Though the confirmation proposal can be created after an attacker's proposal is finalised, if the attacker is fast they can create more of such proposal to further prevent creation of the confirmation proposal.

A secondary impact is that this can prevent the original proposer of the SET_CONTRACT or SET_WEBSITE_URL from creating new proposals. Since proposal finalization and execution occurs in 1 transaction, preventing the creation of the confirmation proposal will also prevent the proposal finalization, which will cause the user to be unable to get rid of their active proposal.

Tools Used

Manual Review

Recommended Mitigation Steps

Check for _confirm suffix in the ballot name whenever proposeSetContractAddress or proposeWebsiteUpdate is called

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/StableConfig.sol#L52 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/StableConfig.sol#L128

Vulnerability details

Impact

The DAO can adjust the collateralisation ratio, minimumCollateralRatioPercent as well as the the percentage of the collateral, the user who call liquidateUser obtains for calling liquidation rewardPercentForCallingLiquidation.

There is a check that the remainingRatioAfterReward which is the remaining ratio of the collateral that goes back to the DAO cannot go lower 105%.

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/StableConfig.sol#L50C1-L56C14

            {
			// Don't increase rewardPercentForCallingLiquidation if the remainingRatio after the rewards would be less than 105% - to ensure that the position will be liquidatable for more than the originally borrowed USDS amount (assume reasonable market volatility)
			uint256 remainingRatioAfterReward = minimumCollateralRatioPercent - rewardPercentForCallingLiquidation - 1;

            if (remainingRatioAfterReward >= 105 && rewardPercentForCallingLiquidation < 10)
                rewardPercentForCallingLiquidation += 1;
            }

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/StableConfig.sol#L128

			uint256 remainingRatioAfterReward = minimumCollateralRatioPercent - 1 - rewardPercentForCallingLiquidation;

            if (remainingRatioAfterReward >= 105 && minimumCollateralRatioPercent > 110)
                minimumCollateralRatioPercent -= 1;
            }

However, the user receives rewardPercentForCallingLiquidation as a percentage of the collateral amount and NOT the loan amount.

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

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

		uint256 rewardedWBTC = (reclaimedWBTC * rewardPercent) / 100;
		uint256 rewardedWETH = (reclaimedWETH * rewardPercent) / 100;

Then the actual remaining ratio after reward can be lower than 105%. For instance, assume we have the following parameters:

1. rewardPercentForCallingLiquidation = 10% (Maximum)

2. minimumCollateralRatioPercent = 116% -> it becomes 115% after we check remainingRatioAfterReward and then lower this.

3. remainingRatioAfterReward = 106% - 10% - 1% = 105%

The actual remaining ratio after reward is actually: 115% - (115% * 10%) = 103.5%

And this instead leaves a buffer of 3.5% for the protocol which is lower than the 5% intended to prevent bad debt.

This can lead the protocol to have a higher risk of bad debt.

Tools Used

Manual Review

Recommended Mitigation Steps

You should use a check that favors the protocol instead

uint256 remainingRatioAfterReward = 105;
...
if (minimumCollateralRatioPercent >= Math.ceilDiv(remainingRatioAfterReward * 100 , 100 - rewardPercentForCallingLiquidation)
...

Note that this check should be done AFTER modifying the parameter.

Assessed type

Math