Salty.IO - t0x1c's results

Lines of code

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

Vulnerability details

Impact

removeLiquidity() has the following code snippet to ensure that reserve0 and reserve1 do not go below DUST:

  185:		// Make sure that removing liquidity doesn't drive either of the reserves below DUST.
  186:		// This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
  187:        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

As can be seen, L187 has a typo where reserve0 is checked twice and reserve1 is completely omitted. Hence, reserve1 can be driven below the DUST threshold and the ratios may not remain constant after a call to removeLiquidity() is made.

Proof of Concept

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

Tools used

Manual inspection

Recommended Mitigation Steps

  185:		// Make sure that removing liquidity doesn't drive either of the reserves below DUST.
  186:		// This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
- 187:        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
+ 187:        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

requiredQuorumForBallotType() calculates minimum quorum by taking into account the live value of staked salt in the pool:

	// The required quorum is normally a default 10% of the amount of SALT staked.
	// There is though a minimum of 0.50% of SALT.totalSupply (in the case that the amount of staked SALT is low - at launch for instance).
	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" );

However, when counting the number of each type of vote in votesCastForBallot() or when counting the total number of votes entered, it makes use of an internal variable:

	function votesCastForBallot( uint256 ballotID, Vote vote ) external view returns (uint256)
		{
		return _votesCastForBallot[ballotID][vote];
		}

This value is not reduced when a user unstakes after casting their vote. This opens up 2 attack vectors for Alice to manipulate the ballot:

Attack Vector - 1 (Manipulation of quorum value)

• Total staked salt = 6,000,000 in the system right now
• Alice stakes her 650,000 salt. The total salt staked is now 6,000,000 + 650,000 = 6,650,000
• A proposal is floated
• Minimum quorum required is 10% of 6,650,000 = 665,000
• Alice votes yes but this is still less than minimum required quorum
• She unstakes her xSalt, reducing total staked salt back to 6,000,000. This is because although the salt is sent back to Alice slowly over a period of time, her staked balance is immediately deducted in full.
• Minimum quorum required now is 10% of 6,000,000 = 600,000
• Alice's yes vote remains recorded and is never removed by the protocol
• Thus, minimum quorum is achieved and proposal is finalized

Alice can now optionally choose to cancel her unstake request.

Attack Vector - 2 (Manipulation of voting power)

Pre-requisite for this attack vector: StakingConfig.minUnstakeWeeks <= ballotMinimumDuration

• Assume StakingConfig.minUnstakeWeeks = 2 weeks and ballotMinimumDuration = 14 days (i.e. 2 weeks)
• Total staked salt = 6,000,000 in the system right now
• Alice stakes her 650,000 salt
• A proposal is floated
• Alice votes yes but this is still less than minimum required quorum. Current count of yes votes = 650,000.
• She unstakes her xSalt providing a duration of StakingConfig.minUnstakeWeeks which is 2 weeks
• Alice receives part of her salt after 2 weeks equalling an amount of 130,000
• She transfers this salt to Bob, which is just another wallet of hers
• Bob stakes 130,000 salt and votes yes
• Thus, Alice increased her voting power and the total count of yes votes now is 650,000 + 130,000 = 780,000

Proof of Concept

Add the following tests inside src/dao/tests/DAO.t.sol and run via COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_unstakingManipulates to see both the tests pass:

	function test_unstakingManipulatesQuorum() public {
        deal(address(salt), address(DEPLOYER), 6_000_000 ether);
		vm.prank(DEPLOYER);
        staking.stakeSALT(6_000_000 ether);
        
        // Set up the parameters for the proposal
        uint256 proposalNum = 0; // Assuming an enumeration starting at 0 for parameter proposals
        uint256 ballotID = 1;

        // Alice stakes her SALT to get voting power
		uint256 aliceStakedAmount = 650_000 ether;
        deal(address(salt), address(alice), aliceStakedAmount);

        vm.startPrank(alice);
        staking.stakeSALT(aliceStakedAmount);

        // Propose a parameter ballot
        proposals.proposeParameterBallot(proposalNum, "Increase max pools count");

		// Alice casts a vote, but not enough for quorum
        proposals.castVote(ballotID, Vote.INCREASE);
		vm.stopPrank();

		skip(11 days);

        // Expect revert because quorum has not yet been reached and do a premature finalization attempt
        // minQuorum required = 10% of (6_000_000 + 650_000) ether = 665_000 ether
        vm.expectRevert("The ballot is not yet able to be finalized");
        dao.finalizeBallot(ballotID);

		// Alice unstakes her SALT
		vm.prank(alice);
		staking.unstake(aliceStakedAmount, 52);

        // Now it should be possible to finalize the ballot without reverting
        // @audit : minQuorum required now = 10% of (6_000_000) ether = 600_000 ether and since Alice had already casted 650_000 votes, so this will pass
        dao.finalizeBallot(ballotID);
        // Check that the ballot is finalized
        bool isBallotFinalized = !proposals.ballotForID(ballotID).ballotIsLive;
        assertTrue(isBallotFinalized);
    }
    
	function test_unstakingManipulatesVotingPower() public {
		// @audit-info : Pre-requisite for the attack `StakingConfig.minUnstakeWeeks <= ballotMinimumDuration`. Either
		// increase `ballotMinimumDuration` to 14 days OR decrease `StakingConfig.minUnstakeWeeks` to 1 week.
		vm.startPrank( address(dao) );
		daoConfig.changeBallotDuration(true);
		daoConfig.changeBallotDuration(true);
		daoConfig.changeBallotDuration(true);
		daoConfig.changeBallotDuration(true);
		assertEq(daoConfig.ballotMinimumDuration(), 14 days, "ballot duration != 14 days");
		vm.stopPrank();

        deal(address(salt), address(DEPLOYER), 6_000_000 ether);
		vm.prank(DEPLOYER);
        staking.stakeSALT(6_000_000 ether);
        
        // Set up the parameters for the proposal
        uint256 proposalNum = 0; // Assuming an enumeration starting at 0 for parameter proposals
        uint256 ballotID = 1;

        // Alice stakes her SALT to get voting power
		uint256 aliceStakedAmount = 650_000 ether;
        deal(address(salt), address(alice), aliceStakedAmount);

        vm.startPrank(alice);
        staking.stakeSALT(aliceStakedAmount);
        // Propose a parameter ballot
        proposals.proposeParameterBallot(proposalNum, "Increase max pools count");
		// Alice casts a vote, but not enough for quorum
        // minQuorum required = 10% of (6_000_000 + 650_000) ether = 665_000 ether
        proposals.castVote(ballotID, Vote.INCREASE);
		assertEq(proposals.votesCastForBallot(ballotID, Vote.INCREASE), aliceStakedAmount);
		vm.stopPrank();

		// Following activities done before anyone calls `finalizeBallot()`
		// Alice partially unstakes her SALT
		vm.startPrank(alice);
		uint256 unstakeID = staking.unstake(aliceStakedAmount, 2); // Alice will receive 130_000 ether of salt back
		skip(14 days);
		staking.recoverSALT(unstakeID); // Alice will receive 130_000 ether of salt back
		salt.transfer(address(bob), 130_000 ether); // Alice transfers the salt to Bob
		vm.stopPrank();
		
		// Bob stakes and casts vote
		vm.startPrank(bob);
		salt.approve(address(staking), 130_000 ether);
		staking.stakeSALT(130_000 ether);
        proposals.castVote(ballotID, Vote.INCREASE);
		vm.stopPrank();
		assertEq(proposals.votesCastForBallot(ballotID, Vote.INCREASE), aliceStakedAmount + 130_000 ether); // @audit : votes incremented
    }

Tools used

Foundry

Recommended Mitigation Steps

• The protocol could add a constraint that any one who has voted in an active ballot shouldn't be able to unstake until that ballot is no more live.
• It may also make sense to store the snapshot of total staked values and voting powers when the proposal is introduced and use those values instead of the live figures.

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

Consider the following scenario:

• Charlie creates a proposal to update website url to https://someNewURL.
• Just a few minutes before 11 days pass, Alice griefs this by creating a new proposal to update the website url to https://someNewURL_confirm. All she has done is append a suffix _confirm at the end of the ballotName here.
• 11 days pass and dao.finalizeBallot() is called which as per current implementation, internally creates a new proposal having the ballotName https://someNewURL_confirm.
• Since a proposal with this ballotName was already created by Alice, we can not finalize for the next 11 days.
• Alice's proposal will have to first pass/fail, then be finalized by DAO so that it is no more in the openBallotsByName mapping. Only after that, can Charlie's ballot be finalized.
• Alice could again choose to grief the re-attempt by DAO to finalize Charlie's ballot (by creating a new proposal exactly like the last time), which again causes another waiting period of 11 days.

The above vulnerability exists not only for proposeWebsiteUpdate() but also for proposeSetContractAddress() since both of them allow providing a user defined ballotName. <br>

Relevant code snippets inside Proposals.sol highlighting the vulnerability:

  240           function proposeSetContractAddress( string calldata contractName, address newAddress, string calldata description ) external nonReentrant returns (uint256 ballotID)
  241                   {
  242                   require( newAddress != address(0), "Proposed address cannot be address(0)" );
  243
  244 @--------->       string memory ballotName = string.concat("setContract:", contractName );
  245                   return _possiblyCreateProposal( ballotName, BallotType.SET_CONTRACT, newAddress, 0, "", description );
  246                   }
  247
  248
  249           function proposeWebsiteUpdate( string calldata newWebsiteURL, string calldata description ) external nonReentrant returns (uint256 ballotID)
  250                   {
  251                   require( keccak256(abi.encodePacked(newWebsiteURL)) != keccak256(abi.encodePacked("")), "newWebsiteURL cannot be empty" );
  252
  253 @--------->       string memory ballotName = string.concat("setURL:", newWebsiteURL );
  254                   return _possiblyCreateProposal( ballotName, BallotType.SET_WEBSITE_URL, address(0), 0, newWebsiteURL, description );
  255                   }

  81            function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 ) internal returns (uint256 ballotID)
  82                    {
  83                    require( block.timestamp >= firstPossibleProposalTimestamp, "Cannot propose ballots within the first 45 days of deployment" );
  84
  85                    // The DAO can create confirmation proposals which won't have the below requirements
  86                    if ( msg.sender != address(exchangeConfig.dao() ) )
  87                            {
  88                            // Make sure that the sender has the minimum amount of xSALT required to make the proposal
  89                            uint256 totalStaked = staking.totalShares(PoolUtils.STAKED_SALT);
  90                            uint256 requiredXSalt = ( totalStaked * daoConfig.requiredProposalPercentStakeTimes1000() ) / ( 100 * 1000 );
  91
  92                            require( requiredXSalt > 0, "requiredXSalt cannot be zero" );
  93
  94                            uint256 userXSalt = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
  95                            require( userXSalt >= requiredXSalt, "Sender does not have enough xSALT to make the proposal" );
  96
  97                            // Make sure that the user doesn't already have an active proposal
  98                            require( ! _userHasActiveProposal[msg.sender], "Users can only have one active proposal at a time" );
  99                            }
  100
  101                   // Make sure that a proposal of the same name is not already open for the ballot
  102                   require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
  103 @--------->       require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );
  104
  105                   uint256 ballotMinimumEndTime = block.timestamp + daoConfig.ballotMinimumDuration();
                        .....
                        .....
                        .....

<br>

Note that in general, a front-running vulnerability exists within the protocol's design. This is because new proposals are not allowed if a previous one with the same ballotName exists. This check is done on ballotName instead of the auto-incrementing ballotId. As such any new proposal can be front-run by a malicious user, hence delaying the process by 11 days or more.

Proof of Concept

Add the following tests inside src/dao/tests/DAO.t.sol and run via COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_grief to see both the tests fail with the reason:

[FAIL. Reason: revert: Cannot create a proposal similar to a ballot that is still open]

<br>

    function test_griefSetWebsite() public
    	{
        vm.startPrank(DEPLOYER);
        staking.stakeSALT( 1000000 ether );
		string memory proposedWebsiteURL = "https://new.com";
        proposals.proposeWebsiteUpdate( proposedWebsiteURL,  "description" );
		assertEq(proposals.ballotForID(1).ballotIsLive, true, "Ballot not correctly created");
        proposals.castVote(1, Vote.YES);
		vm.stopPrank();

		// griefing atack by Alice
		skip(10 days + 23 hours + 55 minutes);
		vm.startPrank(alice);
        staking.stakeSALT( 1000000 ether );
        proposals.proposeWebsiteUpdate( string.concat(proposedWebsiteURL, "_confirm"),  "griefed" );
		vm.stopPrank();

        // Increase block time and try to finalize the ballot
        skip(5 minutes);
        dao.finalizeBallot(1);
    	}

    function test_griefSetContractAddress() public
    	{
        vm.startPrank(DEPLOYER);
        staking.stakeSALT( 1000000 ether );
		string memory contractName = "contractName";
        proposals.proposeSetContractAddress( contractName, address(0x1231236), "description" );
		assertEq(proposals.ballotForID(1).ballotIsLive, true, "Ballot not correctly created");
        proposals.castVote(1, Vote.YES);
		vm.stopPrank();

		// griefing atack by Alice
		skip(10 days + 23 hours + 55 minutes);
		vm.startPrank(alice);
        staking.stakeSALT( 1000000 ether );
        proposals.proposeSetContractAddress( string.concat(contractName, "_confirm"), address(0x1231237), "griefed" );
		vm.stopPrank();

        // Increase block time and try to finalize the ballot
        skip(5 minutes);
        dao.finalizeBallot(1);
    	}

Tools used

Foundry

Recommended Mitigation Steps

Make sure that the sender can not have a ballotName ending in _confirm, if the length of ballotName is greater than 7 letters. Here's where the change needs to be inserted:

	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 can not have a ballotName ending in `_confirm`, if the length of ballotName is greater than 7 letters
+			require(last8lettersAreAsExpected(ballotName), "invalid ballot name");
			// 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);
		}

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Inside DAO.sol, _finalizeTokenWhitelisting() before finalizing a token whitelisting proposal makes a check that it's the ballot with most votes:

249			// Fail to whitelist for now if this isn't the whitelisting proposal with the most votes - can try again later.
250			uint256 bestWhitelistingBallotID = proposals.tokenWhitelistingBallotWithTheMostVotes();
251			require( bestWhitelistingBallotID == ballotID, "Only the token whitelisting ballot with the most votes can be finalized" );

The tokenWhitelistingBallotWithTheMostVotes() function however does not care if the other ballots being looped through are yet not past their deadline. This is an incorrect approach because a ballot which has more Yes votes at this point of time may well turn into a rejected ballot by its deadline timestamp if additional No votes are cast in coming days. Hence using its current state to deny whitelisting of a proposal past its deadline is unfair & only contributes to delaying the timelines, since this may happen again & again which would result in the protocol repeatedly pushing the finalization time further & further away into the future.<br>

• Either only ballots past their deadline should be considered, OR
• Only compare ballots which were created within a few hours of each other <br>

Explanation through an example scenario (also provided in the form of a coded PoC in the next section):

• Dan, Alice and Bob stake some salt. Dan = 2,400,000, Alice = 290,000 and Bob = 310,000
• Hence, total staked salt = 6,000,000
• Alice floats ProposalA for token whitelisting with deadline timestamp t_a
• Minimum quorum required is 20% of 6,000,000 = 600,000
• ProposalA receives votes : YES = 310,000; NO = 290,000. Quorum reached.
• Just a day before t_a, Bob floats ProposalB for whitelisting with deadline timestamp t_b
• ProposalB receives votes : YES = 600,000; NO = 0. Quorum reached. Also, ProposalB has more Yes votes than ProposalA.
• At timestamp t_a, finalizeBallot() is called for ProposalA but it faces a revert on L251 because ProposalB has greater Yes votes
• An hour before t_b, Dan casts his No vote for ProposalB which results in : YES = 600,000; NO = 2,400,000 making it lose with overwhelming majority.
• Note that instead of the above step, another possibility is that some of the current 'Yesvoters change their vote toNo`.
• finalizeBallot() can be called again for ProposalA and it would pass now given that no other proposal has been floated meanwhile which has accumulated more votes, else we could keep going through the above cycle once again.

This caused a delay of around 11 days in ProposalA's finalization. This works as a good attack vector for a malicious staker who wishes to postpone someone's else proposal but does not have enough voting power to defeat it. With some positive votes coming their way from unsuspecting stakers, they can mount this attack. Here's how:

• Alice and Bob stake some salt. Alice = 290,000 and Bob = 310,000. Bob is the malicious actor here.
• Remaining salt is staked by other normal users amounting to 2,400,000
• Hence, total staked salt = 6,000,000
• Alice floats ProposalA for token whitelisting with deadline timestamp t_a
• Minimum quorum required is 20% of 6,000,000 = 600,000
• ProposalA receives votes : YES = 315,000; NO = 310,000. Quorum reached. Bob had voted No, but he could not overcome the Yes votes, getting beat by a small margin.
• Bob attempts his attack. A few days before t_a, Bob floats ProposalB for whitelisting with deadline timestamp t_b and votes Yes himself.
• Some unsuspecting users don't have anything against his proposal and choose to vote Yes. At timestamp t_a his vote tally turns out to be YES = 320,000; NO = 319,990. In spite of being very close to losing the ballot, he still has more Yes votes than ProposalA.
• At timestamp t_a when finalizeBallot() is called for ProposalA, it faces a revert due to having lesser votes. Bob successfully delayed ProposalA's finalization.
• Bob was never really interested in whitelisting any token, so upon reaching close to his deadline t_b, he can choose to change his votes to No and let the proposal fail.

Proof of Concept

Add the following tests inside src/dao/tests/DAO.t.sol and run via COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_tokenWhitelistingDelayed to see the test pass:

	function test_tokenWhitelistingDelayed() public
		{
		// *************** Setup steps ***************
        deal(address(salt), address(DEPLOYER), 2_400_000 ether);
		uint256 aliceStakedAmount = 290_000 ether;
		uint256 bobStakedAmount = 310_000 ether;
        deal(address(salt), address(alice), aliceStakedAmount);
        deal(address(salt), address(bob), bobStakedAmount);
        deal(address(salt), address(dao), 5000000 ether);

		vm.prank(DEPLOYER);
        staking.stakeSALT(2_400_000 ether);
		vm.startPrank(bob);
		salt.approve(address(staking), bobStakedAmount);
        staking.stakeSALT(bobStakedAmount);
		vm.stopPrank();
		// *********************************************

        // Alice stakes her SALT to get voting power
        vm.startPrank(alice);
        staking.stakeSALT(aliceStakedAmount);

		// Propose a whitelisting ballot and cast vote
		// Quorum required = 20% of (2_400_000 + 290_000 + 310_000) = 600_000 
		uint256 ballotID = 1;
		IERC20 test = new TestERC20( "TEST", 18 );
		proposals.proposeTokenWhitelisting(test, "url", "description");
		proposals.castVote(ballotID, Vote.NO);
		vm.stopPrank();
		vm.prank(bob);
		proposals.castVote(ballotID, Vote.YES); // @audit-info : ballot_1 quorum reached

		// Increase block time to 1 hour before finalizing the ballot
		skip(daoConfig.ballotMinimumDuration() - 1 hours);

		// Bob proposes a whitelisting ballot and casts vote
		vm.startPrank(bob);
		IERC20 test2 = new TestERC20( "TEST2", 18 );
		proposals.proposeTokenWhitelisting(test2, "url2", "description2");
		proposals.castVote(ballotID + 1, Vote.YES);
		vm.stopPrank();
		vm.prank(alice);
		proposals.castVote(ballotID + 1, Vote.YES); // @audit-info : ballot_2 quorum reached; All YES votes; deadline still in the future

		// Increase block time to that of finalizing ballot_1
		skip(1 hours);

		// @audit : this would revert since Bob's ballot has more YES votes than Alice's
		vm.expectRevert("Only the token whitelisting ballot with the most votes can be finalized");
        dao.finalizeBallot(ballotID);

		skip(daoConfig.ballotMinimumDuration() - 2 hours);
		// more votes are cast for ballot_2
		vm.prank(DEPLOYER);
		proposals.castVote(ballotID + 1, Vote.NO); // @audit-info : ballot_2 now has NO > YES votes

		// @audit : this would pass now
        dao.finalizeBallot(ballotID);
		// Check that the ballot is finalized
        bool isBallotFinalized = !proposals.ballotForID(ballotID).ballotIsLive;
        assertTrue(isBallotFinalized);
    }

Tools used

Foundry

Recommended Mitigation Steps

• Either only ballots past their deadline should be considered, OR
• Only compare ballots which were created within a few hours of each other.

The following diff uses the first option:

	// Returns the ballotID of the whitelisting ballot that currently has the most yes votes
	// Requires that the quorum has been reached and that the number of yes votes is greater than the number no votes
	function tokenWhitelistingBallotWithTheMostVotes() external view returns (uint256)
		{
		uint256 quorum = requiredQuorumForBallotType( BallotType.WHITELIST_TOKEN);

		uint256 bestID = 0;
		uint256 mostYes = 0;
		for( uint256 i = 0; i < _openBallotsForTokenWhitelisting.length(); i++ )
			{
			uint256 ballotID = _openBallotsForTokenWhitelisting.at(i);
+			if (block.timestamp < ballots[ballotID].ballotMinimumEndTime)
+			    continue;
			uint256 yesTotal = _votesCastForBallot[ballotID][Vote.YES];
			uint256 noTotal = _votesCastForBallot[ballotID][Vote.NO];

			if ( (yesTotal + noTotal) >= quorum ) // Make sure that quorum has been reached
			if ( yesTotal > noTotal )  // Make sure the token vote is favorable
			if ( yesTotal > mostYes )  // Make sure these are the most yes votes seen
				{
				bestID = ballotID;
				mostYes = yesTotal;
				}
			}

		return bestID;
		}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/arbitrage/ArbitrageSearch.sol#L74-L76

Vulnerability details

Impact

The _bisectionSearch() algorithm tries finding the profitable arbitrage point inside the search space by repeating the process 8 times using the loop here on L115:

			// Cost is about 492 gas per loop iteration
			for( uint256 i = 0; i < 8; i++ )
				{
				uint256 midpoint = (leftPoint + rightPoint) >> 1;

				// Right of midpoint is more profitable?
				if ( _rightMoreProfitable( midpoint, reservesA0, reservesA1, reservesB0, reservesB1, reservesC0, reservesC1 ) )
					leftPoint = midpoint;
				else
					rightPoint = midpoint;
				}

Here, the _rightMoreProfitable() function receives the midpoint as an argument and proceeds to calculate the profitMidpoint. It then checks if this is less than DUST amount and returns false in that case:

	// Given the reserves for the arbitrage swap, determine if right of the midpoint looks to be more profitable than the midpoint itself.
	// Used as a substitution for the overly complex derivative in order to determine which direction the optimal arbitrage amountIn is more likely to be.
	function _rightMoreProfitable( uint256 midpoint, uint256 reservesA0, uint256 reservesA1, uint256 reservesB0, uint256 reservesB1, uint256 reservesC0, uint256 reservesC1 ) internal pure returns (bool rightMoreProfitable)
		{
		unchecked
			{
			// Calculate the AMM output of the midpoint
			uint256 amountOut = (reservesA1 * midpoint) / (reservesA0 + midpoint);
			amountOut = (reservesB1 * amountOut) / (reservesB0 + amountOut);
			amountOut = (reservesC1 * amountOut) / (reservesC0 + amountOut);

			int256 profitMidpoint = int256(amountOut) - int256(midpoint);

			// If the midpoint isn't profitable then we can remove the right half the range as nothing there will be profitable there either.
@------->	if ( profitMidpoint < int256(PoolUtils.DUST) )
				return false;


			// Calculate the AMM output of a point just to the right of the midpoint
			midpoint += MIDPOINT_PRECISION;

			amountOut = (reservesA1 * midpoint) / (reservesA0 + midpoint);
			amountOut = (reservesB1 * amountOut) / (reservesB0 + amountOut);
			amountOut = (reservesC1 * amountOut) / (reservesC0 + amountOut);

			int256 profitRightOfMidpoint = int256(amountOut) - int256(midpoint);

			return profitRightOfMidpoint > profitMidpoint;
			}
		}

The comment on L74 mentions that:

    // If the midpoint isn't profitable then we can remove the right half the range as nothing there will be profitable there either.

This is not true and our objective is to show that:

• The entire family of profit function curve values where the first greater-than-dust value occurs to the right of the initial midpoint (blue dotted vertical line) is ignored by the current logic and bestArbAmountIn is returned as zero, even though there exists a valid arbitrage opportunity on the curve. (See example #3 later)
• Even if the starting position of the midpoint is at a comfortable position inside the curve, while doing the search the search space can move towards the left transforming it into a configuration as above (See example #5 later). The alogrithm has no way of recovering and re-considering the right search space again. Hence, it will miss the profit peak and return zero.
• Lastly, we also additionally observe that if the midpoint is too far off to the right, even after 8 halvings (i.e. division by $2^8 = 256$) it does not intersect the profit curve due to which a value of zero is returned (See example #4 later).

While the coded PoC has been supplied in a later section, we'll first try to visualize the problematic configurations by extracting out the core logic of the current implementation and playing around with different swapInAmounts. <br><br>

Here is the Desmos graph calculator which simulates an example profit curve. Legend & variables used:

• The first slider $var_{swapAmountInETH}$ can be used to change values
• The left & right orange lines are the left and right limits of the search space: 1/128 of $var_{swapAmountInETH}$ and 125% of $var_{swapAmountInETH}$ respectively
• The dotted blue vertical line is the midpoint
• The dashed horizontal red line is the DUST threshold, kept as 10 in these examples for easy calculation
• The green curve is the arbitrage profit curve with the peak at $var_{swapAmountInETH} = 40$ (or midpoint of 26.375), yielding a profit of 32.

We will abstract away the core logic of the _bisectionSearch() into a python program so that it's easier to work with decimals and simulate the behaviour represented in the Desmos graph. Here is the code in an online compiler. At the end of the code, one can change the arguments and call print(_bisectionSearch(118)) to see the returned value for a given swapAmountInETH. The output is expected to always be approximately equal to 26.375. An output of 0 means no peak i.e. no arbitrage opportunity was found by the protocol. <br>

This is the abstracted python code version of ArbitrageSearch.sol we will be using:

MIDPOINT_PRECISION = 0.001
DUST = 10

# Desmos curve equation which returns the arbitrage profit at any midpoint value
def _getProfitAtPoint(point):
  return (-0.046 * (point-26.375)**2 + 32)
  
  
def _rightMoreProfitable(midpoint):
  profitMidpoint = _getProfitAtPoint(midpoint)
  if (profitMidpoint < DUST):
    return False
  
  profitRightOfMidpoint = _getProfitAtPoint(midpoint + MIDPOINT_PRECISION)
  
  return profitRightOfMidpoint > profitMidpoint
  
  
def _bisectionSearch(swapAmountInValueInETH):
  leftPoint = swapAmountInValueInETH / 128
  rightPoint = swapAmountInValueInETH + (swapAmountInValueInETH / 4)
  
  for i in range(8):
    midpoint = (leftPoint + rightPoint) / 2
    if _rightMoreProfitable(midpoint):
      leftPoint = midpoint
    else:
      rightPoint = midpoint
      
  bestArbAmountIn = (leftPoint + rightPoint) / 2
  
  if (_getProfitAtPoint(bestArbAmountIn) < DUST):
    return 0
    
  return bestArbAmountIn
  
  
# test for any `swapAmountInValueInETH`
print(_bisectionSearch(40))
print(_bisectionSearch(50))
print(_bisectionSearch(5))
print(_bisectionSearch(9900))

Output:

26.4178466796875
26.471710205078125
0
0

Proof of Concept

Move the slider for $var_{swapAmountInETH}$ on the Desmos calculator to match the values under column swapAmountInETH of the following Table of Examples:

Coded POC

Add the following tests inside src/scenario_tests/Comprehensive1.t.sol and run via COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_ArbExample_ to run both the tests which reproduce the behaviour of Examples# 3 and 4.

	function arbTestsSetup() internal {
		// set the token prices for easy calculation
		vm.startPrank( DEPLOYER );
		forcedPriceFeed.setBTCPrice(1 ether);
		forcedPriceFeed.setETHPrice(1 ether);
		vm.stopPrank();

		// Cast votes for the BootstrapBallot so that the initialDistribution can happen
		bytes memory sig = abi.encodePacked(aliceVotingSignature);
		vm.startPrank(alice);
		bootstrapBallot.vote(true, sig);
		vm.stopPrank();

		sig = abi.encodePacked(bobVotingSignature);
		vm.startPrank(bob);
		bootstrapBallot.vote(true, sig);
		vm.stopPrank();

		// Finalize the ballot to distribute SALT to the protocol contracts and start up the exchange
		vm.warp( bootstrapBallot.completionTimestamp() );
		bootstrapBallot.finalizeBallot();

		deal(address(salt), address(alice), 1e36);
		deal(address(wbtc), address(alice), 1e36);
		deal(address(weth), address(alice), 1e36);
		deal(address(weth), address(bob), 	1e36);

		// No liquidity exists yet
		// Alice adds some SALT/WETH, AND SALT/WBTC
		vm.startPrank(alice);
		salt.approve(address(collateralAndLiquidity), type(uint256).max);
		weth.approve(address(collateralAndLiquidity), type(uint256).max);
		wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
		vm.stopPrank();
	}

	// helper function to calculate the actual profit for for a given point using AMM formula
	function helper_realProfitAtPoint(uint256 swapInQty, uint256 swapPoint, uint256 reserve_weth_pool_1, uint256 reserve_salt_pool_1, uint256 reserve_salt_pool_2, uint256 reserve_wbtc_pool_2, uint256 reserve_wbtc_pool_3, uint256 reserve_weth_pool_3) internal pure returns (uint256) {
		uint swapAmountOut = (swapPoint * 1e18 * reserve_salt_pool_2 * 1e18) / (swapPoint * 1e18 + reserve_wbtc_pool_2 * 1e18);
		uint rsalt2 = reserve_salt_pool_2 * 1e18 - swapAmountOut;
		uint rwbtc2 = reserve_wbtc_pool_2 * 1e18 + swapPoint * 1e18;
		uint out1 = (swapInQty * reserve_salt_pool_1 * 1e18) / (swapInQty + reserve_weth_pool_1 * 1e18);
		uint out2 = (out1  * rwbtc2) / (out1 + rsalt2);
		uint out3 = (out2  * reserve_weth_pool_3 * 1e18) / (out2 + reserve_wbtc_pool_3 * 1e18);
		if (out3 > swapInQty)
			return out3 - swapInQty;
		else
			return 0;
	}

	// reproduces example #3 from the 'table of examples'
	function test_ArbExample_3() public
	{
		arbTestsSetup();

		uint256 reserve_weth_pool_1 = 0.028 ether;
		uint256 reserve_salt_pool_1 = 0.01 ether;
		uint256 reserve_salt_pool_2 = reserve_salt_pool_1;
		uint256 reserve_wbtc_pool_2 = 0.01 ether;
		uint256 reserve_wbtc_pool_3 = reserve_wbtc_pool_2;
		uint256 reserve_weth_pool_3 = reserve_weth_pool_1;

		// Alice adds liquidity
		vm.startPrank(alice);
		collateralAndLiquidity.depositLiquidityAndIncreaseShare(weth, salt,  reserve_weth_pool_1 * 10**18, reserve_salt_pool_1 * 10**18, 0, block.timestamp, false);
		collateralAndLiquidity.depositLiquidityAndIncreaseShare(salt, wbtc,  reserve_salt_pool_2 * 10**18, reserve_wbtc_pool_2 * 10**8, 0, block.timestamp, false);
		collateralAndLiquidity.depositCollateralAndIncreaseShare(reserve_wbtc_pool_3 * 10**8, reserve_weth_pool_3 * 10**18, 0, block.timestamp, false);
		vm.stopPrank();

		// Bob swaps
		vm.startPrank(bob);
		salt.approve(address(collateralAndLiquidity), type(uint256).max);
		uint swapPoint = 33;
		uint swapAmountIn = swapPoint * 10**8;
		pools.depositSwapWithdraw(wbtc, salt, swapAmountIn, 0, block.timestamp);
		console.log("\ntest_ArbExample_3:");
		// @audit : 0 
		console.log( "Protocol calculated ARBITRAGE PROFITS: (weth)", pools.depositedUserBalance( address(dao), weth ) );
		// @audit : 58560
		console.log( "AMM formula calculated real profit at point 10.748437 =", helper_realProfitAtPoint(10.748437 ether, swapPoint, reserve_weth_pool_1, reserve_salt_pool_1, reserve_salt_pool_2, reserve_wbtc_pool_2, reserve_wbtc_pool_3, reserve_weth_pool_3));
	}

	// reproduces example #4 from the 'table of examples'
	function test_ArbExample_4() public
	{
		arbTestsSetup();

		uint256 reserve_weth_pool_1 = 10;
		uint256 reserve_salt_pool_1 = 10;
		uint256 reserve_salt_pool_2 = reserve_salt_pool_1;
		uint256 reserve_wbtc_pool_2 = 10;
		uint256 reserve_wbtc_pool_3 = reserve_wbtc_pool_2;
		uint256 reserve_weth_pool_3 = reserve_weth_pool_1;

		// Alice adds liquidity
		vm.startPrank(alice);
		collateralAndLiquidity.depositLiquidityAndIncreaseShare(weth, salt,  reserve_weth_pool_1 * 10**18, reserve_salt_pool_1 * 10**18, 0, block.timestamp, false);
		collateralAndLiquidity.depositLiquidityAndIncreaseShare(salt, wbtc,  reserve_salt_pool_2 * 10**18, reserve_wbtc_pool_2 * 10**8, 0, block.timestamp, false);
		collateralAndLiquidity.depositCollateralAndIncreaseShare(reserve_wbtc_pool_3 * 10**8, reserve_weth_pool_3 * 10**18, 0, block.timestamp, false);
		vm.stopPrank();

		// Bob swaps
		vm.startPrank(bob);
		salt.approve(address(collateralAndLiquidity), type(uint256).max);
		uint swapPoint = 990;
		uint swapAmountIn = swapPoint * 10**8;
		pools.depositSwapWithdraw(wbtc, salt, swapAmountIn, 0, block.timestamp);
		console.log("\ntest_ArbExample_4:");
		// @audit : 0 ether
		console.log( "Protocol calculated ARBITRAGE PROFITS: (weth)", pools.depositedUserBalance( address(dao), weth ) );
		// @audit : 2.164 ether
		console.log( "AMM formula calculated real profit at point 7.734 =", helper_realProfitAtPoint(7.734 ether, swapPoint, reserve_weth_pool_1, reserve_salt_pool_1, reserve_salt_pool_2, reserve_wbtc_pool_2, reserve_wbtc_pool_3, reserve_weth_pool_3));
	}

Output:

[PASS] test_ArbExample_3() (gas: 2005727)
Logs:
  
test_ArbExample_3:
  Protocol calculated ARBITRAGE PROFITS: (weth) 0
  AMM formula calculated real profit at point 10.748437 = 58560

[PASS] test_ArbExample_4() (gas: 2005762)
Logs:
  
test_ArbExample_4:
  Protocol calculated ARBITRAGE PROFITS: (weth) 0
  AMM formula calculated real profit at point 7.734 = 2164742798229448455

Tools used

Foundry and manual inspection.

Recommended Mitigation Steps

We would have to rethink the current logic. One way to handle this could be -

• Proceed as usual, but save the initial/first midpoint value in memory as savedMidpoint. We'll use this later.
• If no peak is eventually returned by the algorithm, then consider the new search space to be such that leftPoint = savedMidpoint and rightPoint = 125% of swapAmountInValueInETH.
• Redo the search.

The above approach is admittedly more expensive. <br>

It would perhaps be better to implement a different search algorithm altogether more suited for the job.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

There are two issues intertwined here which need to be highlighted:

• 1. Malicious users can bypass minimumCollateralValueForBorrowing threshold by first taking out a valid loan, then repaying partially such that the remaining position is too small to be of interest to any liquidator.
• 2. Due to the way maxWithdrawableCollateral() and userCollateralValueInUSD() are calculated, it allows users to call withdrawCollateralAndClaim() even when the remaining loan amount becomes undercollateralized.

Details

StableConfig.sol quite correctly states in its comments that:

            // The minimum USD value of collateral - to borrow an initial amount of USDS.
@------->   // This is to help prevent saturation of the contract with small amounts of positions and to ensure that liquidating the position yields non-trivial rewards
            // Range: 1000 to 5000 with an adjustment of 500 ether
            uint256 public minimumCollateralValueForBorrowing = 2500 ether;

Preventing such small positions is important since this can be used by a malicious competitor in the following manner -

• A malicious whale/competitor/group-of-actors open a large short position against the protocol (or simply want to bring the protocol down with bad debt).
• They create multiple small positions/loans which are just not profitable for any liquidator to liquidate. This is because any liquidator has to receive rewards greater than the gas fee they have to pay for calling the liquidateUser function.
• In the end these low value loans will never get liquidated, leaving the protocol with bad debt and can even cause the protocol to go underwater.

See a similar issue raised in the past rated as high impact & high likelihood. It additionally highlights in the comments section how this can become an attack vector (even by non-whales) on chains which aren't costly. <br><br>

The issue at hand is that although there is a check present while calling borrowUSDS() via an internal call to maxBorrowableUSDS() to prevent loans smaller than minimumCollateralValueForBorrowing which is set by default to 2500 ether, there is no such check when using repayUSDS() to pay off the loan partially. Hence, any malicious user can choose to repay all except a small amount of their debt. The remaining debt would not be profitable enough for any liquidator. This is not just because of gas fees, but also because the malicuous actor can craft this in such a way that the remaining collateral value expressed in USD equals zero, and hence no rewards would be payable to the liquidator at all. This is where the second issue of withdrawCollateralAndClaim() and in turn userCollateralValueInUSD() comes into play. <br>

L84 allows collateral withdrawal if it is not more than maxWithdrawableCollateral(msg.sender). This internally calls userCollateralValueInUSD() which has the following lines:

	function userCollateralValueInUSD( address wallet ) public view returns (uint256)
		{
@--->	uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );
		if ( userCollateralAmount == 0 )
			return 0;

		uint256 totalCollateralShares = totalShares[collateralPoolID];

		// Determine how much collateral share the user currently has
		(uint256 reservesWBTC, uint256 reservesWETH) = pools.getPoolReserves(wbtc, weth);

@--->	uint256 userWBTC = (reservesWBTC * userCollateralAmount ) / totalCollateralShares;
@--->	uint256 userWETH = (reservesWETH * userCollateralAmount ) / totalCollateralShares;

		return underlyingTokenValueInUSD( userWBTC, userWETH );
		}

This works fine the first time when user's eligibility for collateral withdrawal is checked and the protocol gives the caller a withdrawable amount which would be "safe" to withdraw without putting the remaining loan in danger of being undercollateralized. However after withdrawal, the remaining userCollateralAmount could be such a small value that userWBTC and userWETH round-down to zero - this is overlooked by the protocol. This results in the behaviour that when the remaining loan is checked, it is found to be canUserBeLiquidated = true even though it was calculated by the protocol before withdrawal that the loan would remain healthy. An example of this is presented in the PoC where Alice leaves behind 1 in her loan (which was allowed by the protocol) and yet when collateral withdrawal has been made, the remaining loan is seen as undercollateralized. This remaining collateral of 1 evaluates to 0 USD value as per the protocol. <br>

This 1 continues to remain in the pool, diluting the value of other users' shares. The following PoC shows the aforementioned behaviour. This action can be repeated by multiple wallets to flood the protocol with bad debt.

Proof of Concept

Add the following test inside src/stable/tests/CollateralAndLiquidity.t.sol and run via COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_createSmallPosition:<br>

	function test_createSmallPosition() public {
        // No loans taken by Alice & Bob initially
        assertEq( collateralAndLiquidity.userShareForPool( alice, collateralPoolID ), 0);
        assertEq( collateralAndLiquidity.userShareForPool( bob, collateralPoolID ), 0);
        assertEq( usds.balanceOf(alice), 0);
        assertEq( usds.balanceOf(bob), 0);

        uint256 collateral = 5000 ether;
        deal(address(wbtc), address(alice), collateral);
        deal(address(weth), address(alice), collateral);
        deal(address(wbtc), address(bob), collateral);
        deal(address(weth), address(bob), collateral);

        // Alice and Bob each deposit and borrow
        vm.startPrank( alice );
		collateralAndLiquidity.depositCollateralAndIncreaseShare(collateral, collateral, 0, block.timestamp, false );
		uint256 maxUSDSAlice = collateralAndLiquidity.maxBorrowableUSDS(alice);
		collateralAndLiquidity.borrowUSDS( maxUSDSAlice );
		vm.stopPrank();

        vm.startPrank( bob );
		collateralAndLiquidity.depositCollateralAndIncreaseShare(collateral, collateral, 0, block.timestamp, false );
		uint256 maxUSDSBob = collateralAndLiquidity.maxBorrowableUSDS(bob);
		collateralAndLiquidity.borrowUSDS( maxUSDSBob );
		vm.stopPrank();

    	skip(1 days );
        assertEq( collateralAndLiquidity.userShareForPool( alice, collateralPoolID ), 2*collateral);
        assertEq( collateralAndLiquidity.userShareForPool( bob, collateralPoolID ), 2*collateral);
        uint256 aliceBorrowedAmount = usds.balanceOf(alice);
        uint256 bobBorrowedAmount = usds.balanceOf(bob);
        assertEq(aliceBorrowedAmount, maxUSDSAlice);
        assertEq(bobBorrowedAmount, maxUSDSBob);

        // Check that Alice can not be liquidated
        assertFalse(collateralAndLiquidity.canUserBeLiquidated(alice)); 
        // Alice repays all her debt except a small amount of 939; and then removes as much collateral as possible
        vm.startPrank(alice);
        collateralAndLiquidity.repayUSDS(aliceBorrowedAmount - 939);
        assertEq(usds.balanceOf(alice), 939);
        collateralAndLiquidity.withdrawCollateralAndClaim(2*collateral - 1, 0, 0, block.timestamp);
        vm.stopPrank();

        // Check collateral.numberOfUsersWithBorrowedUSDS returns correct number of open positions
        assertEq(collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 2);

        // Check if Alice can be liquidated
        assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice)); // @audit : Alice was allowed to withdraw collateral even when it resulted in loan becoming undercollateralized

        // Check Alice's borrowed & collateral amounts
        assertTrue(_userHasCollateral(alice));
        assertEq(collateralAndLiquidity.usdsBorrowedByUsers(alice), 939);
        assertEq(collateralAndLiquidity.userShareForPool( alice, collateralPoolID ), 1);
        assertEq(collateralAndLiquidity.userCollateralValueInUSD( alice ), 0, "collateral value in USD > 0");

        /*
        // There is no incentive for anyone to liquidate Alice now since the reward is 0
        skip(1 days);
        vm.prank(bob);
        vm.expectEmit(false, false, false, false, address(collateralAndLiquidity));
        emit Liquidation(address(bob), address(alice), 0, 0, 939);
        collateralAndLiquidity.liquidateUser(alice);
        */
    }

Tools Used

Foundry

Recommended Mitigation Steps

Add a check inside repayUSDS that if the entire loan is not being settled, then the partial repayment should not push the collateral value below a certain limit. This limit could be minimumCollateralValueForBorrowing or some other value. Also, the userWBTC & userWETH calculated should be verified after calculation so that "undercollateralized withdrawal" can be avoided.

Note

Although the two issues feel to be distinct to me, each requiring a separate fix, omitting any one from the report would have resulted in the full impact not being highighted properly and hence have clubbed them together. I leave it to the judges to decide if these need to be considered separately or as one.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/rewards/RewardsEmitter.sol#L103

Vulnerability details

Impact

RewardsEmitter::performUpkeep() distributes the added rewards to the eligible staking pools at the rate of X% per day (default value set to 1% by the protocol). To ensure that once disbursed, it is not sent again, the code reduces the pendingRewards[poolID] variable on L126:

120			// Each pool will send a percentage of the pending rewards based on the time elapsed since the last send
121			uint256 amountToAddForPool = ( pendingRewards[poolID] * numeratorMult ) / denominatorMult;
122
123			// Reduce the pending rewards so they are not sent again
124			if ( amountToAddForPool != 0 )
125				{
126				pendingRewards[poolID] -= amountToAddForPool;
127
128				sum += amountToAddForPool;
129				}

The impact of this is: higher the number of times performUpkeep() is called, lesser the rewards per day is distributed to the pools. That is, calling it 100 times a day is worse than calling it once at the end of the day. This is at odds with how the protocol wants to achieve a higher frequency of upkeep-ing. <br>

Reasoning:<br> This above code logic is incorrect maths as doing this will result in the following scenario:

• Suppose that on Day0, the addedRewards = 10 ether and rewardsEmitterDailyPercentTimes1000 = 2500 i.e. 2.5% per day. One would expect all rewards to be distributed to the pool after 40 days (since 2.5% * 40 = 100%).
• On Day1, performUpkeep() gets called. amountToAddForPool and pendingRewards[poolID] are calculated on L121 & L126 respectively as:
  • amountToAddForPool = 0.025 * 10 ether = 0.25 ether
  • pendingRewards[poolID] = 10 ether - 0.25 ether = 9.75 ether
• On Day2, performUpkeep() gets called again. amountToAddForPool and pendingRewards[poolID] are calculated now as:
  • amountToAddForPool = 0.025 * 9.75 ether = 0.24375 ether
  • pendingRewards[poolID] = 9.75 ether - 0.24375 ether = 9.50625 ether

So on and so forth for each new day. The actual formula being followed is totalRewardsStillRemainingAfterXdays = 10 ether * (1 - 2.5%)**X which would be 3632324398878806621 or 3.63232 ether after 40 days. In fact, even after another 40 days, it's still not all paid out. Please refer the "Proof of Concept - 1" provided below. In fact, since this is a classic negative compounding formula ( $Amount = Principle * (1 - rate)^{time}$ ), no matter how many days pass, all the rewards would never be distributed and dust would always remain in the contract.

Further Evidence & Impact

To remove any doubts regarding the interpretation of the 1% per day rate and if indeed the current implementation is as intended or not, one can look at the following calculation provided in the comments:<br> StakingRewards.sol#L178-L181

	// 3. Rewards are first placed into a RewardsEmitter which deposits rewards via addSALTRewards at the default rate of 1% per day.
	// 4. Rewards are deposited fairly often, with outstanding rewards being transferred with a frequency proportional to the activity of the exchange.
	// Example: if $100k rewards were being deposited in a bulk transaction, it would only equate to $1000 (1%) the first day,
	// or $10 in claimable rewards during a 15 minute upkeep period.

Let's crunch the above numbers -

• The comment says, "$100K rewards would equate to $1000 (1%) the first day" OR "$10 claimable rewards" when upkeep() is called every 15 minutes.
• This conclusion could have been arrived at by the protocol only when the reward of $1000 (over 24 hours) is divided equally for each 15 minutes i.e. $1000 / (24 * 60 / 15) equalling $10 (rounded-down).

This is certainly not the case currently as can be seen in the "Proof of Concept - 2". <br><br>

Note that this means that per day much lesser rewards than 1% are distributed if performUpkeep() is called every 15 minutes instead of once after 24 hours. A malicious actor can use this to grief the protocol and delay/diminish emissions. The power of negative compunding causes this and can be seen in the second PoC.

Proof of Concept - 1

Add the following test inside src/rewards/tests/RewardsEmitter.t.sol and run via COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_allPendingRewardsNeverPaidOut to see the two assertions fail. The value of rewardsEmitterDailyPercentTimes1000 in the test is 2.5% per day.<br>

    function test_allPendingRewardsNeverPaidOut() public {

        // Add some pending rewards to the pools
        AddedReward[] memory addedRewards = new AddedReward[](1);
        addedRewards[0] = AddedReward({poolID: poolIDs[0], amountToAdd: 10 ether});
        liquidityRewardsEmitter.addSALTRewards(addedRewards);

        // Verify that the rewards were added
        assertEq(pendingLiquidityRewardsForPool(poolIDs[0]), 10 ether);

        // Call performUpkeep
        vm.startPrank(address(upkeep));
        for (uint256 i; i < 40; ++i)
            liquidityRewardsEmitter.performUpkeep(1 days);
        vm.stopPrank();

        // Verify that the correct amount of rewards were deducted from each pool's pending rewards
        // 2.5% of the rewards should be deducted per day, so all rewards should be paid out after the 40 days' iteration above
        assertEq(pendingLiquidityRewardsForPool(poolIDs[0]), 0 ether, "not all reward distributed");


        // Let's try 40 times more, just to confirm the behaviour and see if ever all the rewards are paid out
        vm.startPrank(address(upkeep));
        for (uint256 i; i < 40; ++i)
            liquidityRewardsEmitter.performUpkeep(1 days);
        vm.stopPrank();
        assertEq(pendingLiquidityRewardsForPool(poolIDs[0]), 0 ether, "nope, not even now");
    }

Output:

[FAIL. Reason: assertion failed] test_allPendingRewardsNeverPaidOut() (gas: 5441491)
Logs:
  Error: not all reward distributed
  Error: a == b not satisfied [uint]
        Left: 3632324398878806621
       Right: 0
  Error: nope, not even now
  Error: a == b not satisfied [uint]
        Left: 1319378053869028394
       Right: 0

Proof of Concept - 2

Add the following test inside src/rewards/tests/RewardsEmitter.t.sol and run via COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_perDayDefaultRateNotAdheredTo to see last assertion fail. The value of rewardsEmitterDailyPercentTimes1000 in the test is 1% per day and upkeep() is called every 15 minutes.<br> We observe that by the end of the day, 4931 ethers less is disbursed than expected.

    function test_perDayDefaultRateNotAdheredTo() public {
        // set daily rate to 1%
        vm.startPrank(address(dao));
		for ( uint256 i = 0; i < 6; i++ )
			rewardsConfig.changeRewardsEmitterDailyPercent(false);
		vm.stopPrank();
        assertEq(rewardsConfig.rewardsEmitterDailyPercentTimes1000(), 1000);

        // Add some pending rewards to the pools
        deal(address(salt), address(this), 100_000_000 ether);
        AddedReward[] memory addedRewards = new AddedReward[](1);
        addedRewards[0] = AddedReward({poolID: poolIDs[0], amountToAdd: 100_000_000 ether});
        liquidityRewardsEmitter.addSALTRewards(addedRewards);
        // Verify that the rewards were added
        assertEq(pendingLiquidityRewardsForPool(poolIDs[0]), 100_000_000 ether);

        // Call performUpkeep every 15 minutes for a full day
        uint256 totalDisbursed = 0;
        uint256 disbursedInLast15Mins = pendingLiquidityRewardsForPool(poolIDs[0]);
        console.log("Time Lapsed ( in mins)        |       Reward emiited in last 15 mins      |       Total reward emitted");
        vm.startPrank(address(upkeep));
        for (uint256 i; i < 24 * 4; ++i) {
            liquidityRewardsEmitter.performUpkeep(15 minutes);
            uint256 diff = disbursedInLast15Mins - pendingLiquidityRewardsForPool(poolIDs[0]);
            totalDisbursed += diff;
            console.log("%s                        |          %s         |          %s", (i+1)*15, diff, totalDisbursed); 
            disbursedInLast15Mins = pendingLiquidityRewardsForPool(poolIDs[0]);
        }
        vm.stopPrank();

        // Verify that the correct amount of rewards were deducted from each pool's pending rewards
        // 1% of the rewards should be deducted per day
        assertEq(pendingLiquidityRewardsForPool(poolIDs[0]), 99_000_000 ether, "1% per day not adhered to");
    }

Output:

[FAIL. Reason: assertion failed] test_perDayDefaultRateNotAdheredTo() (gas: 7243619)
Logs:
  Time Lapsed ( in mins)        |       Reward emiited in last 15 mins      |       Total reward emitted
  15                        |          10416666666666666666666         |          10416666666666666666666
  30                        |          10415581597222222222222         |          20832248263888888888888
  45                        |          10414496640805844907407         |          31246744904694733796295
  60                        |          10413411797405760965229         |          41660156702100494761524
  75                        |          10412327067010197865129         |          52072483769110692626653
  90                        |          10411242449607384302851         |          62483726218718076929504
  105                        |          10410157945185550200319         |          72893884163903627129823
  120                        |          10409073553732926705507         |          83302957717636553835330
  135                        |          10407989275237746192308         |          93710946992874300027638
  150                        |          10406905109688242260413         |          104117852102562542288051
  165                        |          10405821057072649735178         |          114523673159635192023229
  180                        |          10404737117379204667497         |          124928410277014396690726
  195                        |          10403653290596144333678         |          135332063567610541024404
  210                        |          10402569576711707235309         |          145734633144322248259713
  225                        |          10401485975714133099139         |          156136119120036381358852
  240                        |          10400402487591662876941         |          166536521607628044235793
  255                        |          10399319112332538745392         |          176935840719960582981185
  270                        |          10398235849925004105939         |          187334076569885587087124
  285                        |          10397152700357303584678         |          197731229270242890671802
  300                        |          10396069663617683032221         |          208127298933860573704023
  315                        |          10394986739694389523572         |          218522285673554963227595
  330                        |          10393903928575671357997         |          228916189602130634585592
  345                        |          10392821230249778058897         |          239309010832380412644489
  360                        |          10391738644704960373682         |          249700749477085373018171
  375                        |          10390656171929470273643         |          260091405649014843291814
  390                        |          10389573811911560953823         |          270480979460926404245637
  405                        |          10388491564639486832891         |          280869471025565891078528
  420                        |          10387409430101503553012         |          291256880455667394631540
  435                        |          10386327408285867979725         |          301643207863953262611265
  450                        |          10385245499180838201811         |          312028453363134100813076
  465                        |          10384163702774673531165         |          322412617065908774344241
  480                        |          10383082019055634502672         |          332795699084964408846913
  495                        |          10382000448011982874078         |          343177699532976391720991
  510                        |          10380918989631981625862         |          353558618522608373346853
  525                        |          10379837643903894961109         |          363938456166512268307962
  540                        |          10378756410815988305384         |          374317212577328256613346
  555                        |          10377675290356528306602         |          384694887867684784919948
  570                        |          10376594282513782834904         |          395071482150198567754852
  585                        |          10375513387276020982525         |          405446995537474588737377
  600                        |          10374432604631513063673         |          415821428142106101801050
  615                        |          10373351934568530614395         |          426194780076674632415445
  630                        |          10372271377075346392456         |          436567051453749978807901
  645                        |          10371190932140234377207         |          446938242385890213185108
  660                        |          10370110599751469769459         |          457308352985641682954567
  675                        |          10369030379897328991358         |          467677383365539011945925
  690                        |          10367950272566089686255         |          478045333638105101632180
  705                        |          10366870277746030718579         |          488412203915851132350759
  720                        |          10365790395425432173713         |          498777994311276564524472
  735                        |          10364710625592575357862         |          509142704936869139882334
  750                        |          10363630968235742797928         |          519506335905104882680262
  765                        |          10362551423343218241387         |          529868887328448100921649
  780                        |          10361471990903286656153         |          540230359319351387577802
  795                        |          10360392670904234230460         |          550590751990255621808262
  810                        |          10359313463334348372728         |          560950065453589970180990
  825                        |          10358234368181917711439         |          571308299821771887892429
  840                        |          10357155385435232095011         |          581665455207207119987440
  855                        |          10356076515082582591667         |          592021531722289702579107
  870                        |          10354997757112261489314         |          602376529479401964068421
  885                        |          10353919111512562295409         |          612730448590914526363830
  900                        |          10352840578271779736837         |          623083289169186306100667
  915                        |          10351762157378209759781         |          633435051326564515860448
  930                        |          10350683848820149529597         |          643785735175384665390045
  945                        |          10349605652585897430688         |          654135340827970562820733
  960                        |          10348527568663753066372         |          664483868396634315887105
  975                        |          10347449597042017258761         |          674831317993676333145866
  990                        |          10346371737708992048630         |          685177689731385325194496
  1005                        |          10345293990652980695292         |          695522983722038305889788
  1020                        |          10344216355862287676469         |          705867200077900593566257
  1035                        |          10343138833325218688170         |          716210338911225812254427
  1050                        |          10342061423030080644556         |          726552400334255892898983
  1065                        |          10340984124965181677823         |          736893384459221074576806
  1080                        |          10339906939118831138064         |          747233291398339905714870
  1095                        |          10338829865479339593154         |          757572121263819245308024
  1110                        |          10337752904035018828613         |          767909874167854264136637
  1125                        |          10336676054774181847485         |          778246550222628445984122
  1140                        |          10335599317685142870209         |          788582149540313588854331
  1155                        |          10334522692756217334494         |          798916672233069806188825
  1170                        |          10333446179975721895188         |          809250118413045528084013
  1185                        |          10332369779331974424157         |          819582488192377502508170
  1200                        |          10331293490813294010155         |          829913781683190796518325
  1215                        |          10330217314408000958696         |          840243998997598797477021
  1230                        |          10329141250104416791929         |          850573140247703214268950
  1245                        |          10328065297890864248513         |          860901205545594078517463
  1260                        |          10326989457755667283487         |          871228195003349745800950
  1275                        |          10325913729687151068145         |          881554108733036896869095
  1290                        |          10324838113673641989909         |          891878946846710538859004
  1305                        |          10323762609703467652202         |          902202709456414006511206
  1320                        |          10322687217764956874321         |          912525396674178963385527
  1335                        |          10321611937846439691314         |          922847008612025403076841
  1350                        |          10320536769936247353846         |          933167545381961650430687
  1365                        |          10319461714022712328080         |          943487007095984362758767
  1380                        |          10318386770094168295545         |          953805393866078531054312
  1395                        |          10317311938138950153015         |          964122705804217481207327
  1410                        |          10316237218145394012374         |          974438943022362875219701
  1425                        |          10315162610101837200497         |          984754105632464712420198
  1440                        |          10314088113996618259122         |          995068193746461330679320
  Error: 1% per day not adhered to
  Error: a == b not satisfied [uint]
        Left: 99004931806253538669320680
       Right: 99000000000000000000000000

Tools Used

Foundry

Recommended Mitigation Steps

Store the eligible pool reward & the already paid out reward in separate variables and compare them to make sure extra rewards are not being paid out to a pool. Irrespective of the performUpkeep() calling frequency, at the end of the day, 1% should be disbursed.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L191-L192

Vulnerability details

Impact

_zapSwapAmount() assumes that:

191:		// r1 * z0 guaranteed to be greater than r0 * z1 per the conditional check in _determineZapSwapAmount
192:        uint256 C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 );

The protocol's assumption of r1 * z0 guaranteed to be greater than r0 * z1 is based on the following check in _determineZapSwapAmount():

214:		// zapAmountA / zapAmountB exceeds the ratio of reserveA / reserveB? - meaning too much zapAmountA
215:		if ( zapAmountA * reserveB > reserveA * zapAmountB )
216:			(swapAmountA, swapAmountB) = (_zapSwapAmount( reserveA, reserveB, zapAmountA, zapAmountB ), 0);
217:
218:		// zapAmountA / zapAmountB is less than the ratio of reserveA / reserveB? - meaning too much zapAmountB
219:		if ( zapAmountA * reserveB < reserveA * zapAmountB )
220:			(swapAmountA, swapAmountB) = (0, _zapSwapAmount( reserveB, reserveA, zapAmountB, zapAmountA ));

The assumption would had been true for all cases if not for this piece of logic inside _zapSwapAmount()#L158-L168 which right shifts the arguments if the maximumMSB is greater than 80:

    	// Assumes the largest number has more than 80 bits - but if not then shifts zero effectively as a straight assignment.
    	// C will be calculated as: C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 );
    	// Multiplying three 80 bit numbers will yield 240 bits - within the 256 bit limit.
		if ( maximumMSB > 80 )
			shift = maximumMSB - 80;

    	// Normalize the inputs to 80 bits.
    	uint256 r0 = reserve0 >> shift;
		uint256 r1 = reserve1 >> shift;
		uint256 z0 = zapAmount0 >> shift;
		uint256 z1 = zapAmount1 >> shift;

This can lead to z0 being reduced to 0 and hence causing underflow on L192. <br>

Since _determineZapSwapAmount() is internally called whenever depositLiquidityAndIncreaseShare() is called with useZapping = true, it will cause a revert when a situation like the following exists:

        uint256 reserveA = 1500000000;
        uint256 reserveB = 2000000000 ether;
        uint256 zapAmountA = 150;
        uint256 zapAmountB = 100 ether;

maximumMSB in this case is 90 (for reserveB) and also an excess of zapAmountA is being provided as compared to the existing reserve ratio. Hence, right shift by 90 - 80 = 10 bits will occur resulting z0 to be 0.

Proof of Concept

Create a new file src/stable/tests/BugPoolMath.t.sol and add the following code. Run it via forge test -vv --mt test_poolMath to see the test revert:

// SPDX-License-Identifier: Unlicensed
pragma solidity =0.8.22;

import "../../pools/PoolMath.sol";
import { console, Test } from "forge-std/Test.sol";

contract BugPoolMath is Test
{
    function test_poolMath() public view {
        uint256 reserveA = 1500000000;
        uint256 reserveB = 2000000000 ether;
        uint256 zapAmountA = 150;
        uint256 zapAmountB = 100 ether;

        (uint256 swapAmountA, uint256 swapAmountB) = PoolMath._determineZapSwapAmount(reserveA, reserveB, zapAmountA, zapAmountB);
        console.log("swapAmountA = %s, swapAmountB = %s", swapAmountA, swapAmountB);
    }
}

Tools used

Foundry

Recommended Mitigation Steps

Make sure to check the assertion again:

    function _zapSwapAmount( uint256 reserve0, uint256 reserve1, uint256 zapAmount0, uint256 zapAmount1 ) internal pure returns (uint256 swapAmount)
    	{
    	uint256 maximumMSB = _maximumMSB( reserve0, reserve1, zapAmount0, zapAmount1);

		uint256 shift = 0;

    	// Assumes the largest number has more than 80 bits - but if not then shifts zero effectively as a straight assignment.
    	// C will be calculated as: C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 );
    	// Multiplying three 80 bit numbers will yield 240 bits - within the 256 bit limit.
		if ( maximumMSB > 80 )
			shift = maximumMSB - 80;

    	// Normalize the inputs to 80 bits.
    	uint256 r0 = reserve0 >> shift;
		uint256 r1 = reserve1 >> shift;
		uint256 z0 = zapAmount0 >> shift;
		uint256 z1 = zapAmount1 >> shift;

		// In order to swap and zap, require that the reduced precision reserves and one of the zapAmounts exceed DUST.
		// Otherwise their value was too small and was crushed by the above precision reduction and we should just return swapAmounts of zero so that default addLiquidity will be attempted without a preceding swap.
        if ( r0 < PoolUtils.DUST)
        	return 0;

        if ( r1 < PoolUtils.DUST)
        	return 0;

        if ( z0 < PoolUtils.DUST)
        if ( z1 < PoolUtils.DUST)
        	return 0;

        // Components of the quadratic formula mentioned in the initial comment block: x = [-B + sqrt(B^2 - 4AC)] / 2A
		uint256 A = 1;
        uint256 B = 2 * r0;

		// Here for reference
//        uint256 C = r0 * ( r0 * z1 - r1 * z0 ) / ( r1 + z1 );
//        uint256 discriminant = B * B - 4 * A * C;

-		// Negate C (from above) and add instead of subtract.
-		// r1 * z0 guaranteed to be greater than r0 * z1 per the conditional check in _determineZapSwapAmount
-       uint256 C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 );
-       uint256 discriminant = B * B + 4 * A * C;
+       uint256 C;
+       uint256 discriminant;
+       if ((r1 * z0) >= (r0 * z1)) {
+           C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 );
+           discriminant = B * B + 4 * A * C;
+       } else {
+           C = r0 * ( r0 * z1 - r1 * z0 ) / ( r1 + z1 );
+           discriminant = B * B - 4 * A * C;
+       }

        // Compute the square root of the discriminant.
        uint256 sqrtDiscriminant = Math.sqrt(discriminant);

		// Safety check: make sure B is not greater than sqrtDiscriminant
		if ( B > sqrtDiscriminant )
			return 0;

        // Only use the positive sqrt of the discriminant from: x = (-B +/- sqrtDiscriminant) / 2A
		swapAmount = ( sqrtDiscriminant - B ) / ( 2 * A );

		// Denormalize from the 80 bit representation
		swapAmount <<= shift;
    	}

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

changeMinimumCollateralRatioPercent() allows the owner to change the minimumCollateralRatioPercent while the changeRewardPercentForCallingLiquidation() function allows to change the rewardPercentForCallingLiquidation. <br> The protocol aims to maintain a remaining ratio of 105% as is evident by the comments in these 2 functions:

// Don't decrease the minimumCollateralRatioPercent 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)

and

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

That is, if borrow amount is 100, after the calls to any of these two functions, there should be at least 105 collateral remaining which will act as buffer against market volatility.<br> The current calculation however is incorrect and there is really no direct relationship (in the way the developer assumes) between the rewardPercentForCallingLiquidation and minimumCollateralRatioPercent. Consider this:

• minimumCollateralRatioPercent of 110% means that for a borrow amount of 200, collateral should not go below 220. This is 110% of 200.
• However, rewardPercentForCallingLiquidation of 5% is calculated on the collateral amount and NOT the borrowed amount as is evident in the comments too here and here. So the liquidator will receive 5% of 220 (let's assume boundary values for rounded calculations) which is 11.
• The remaining amount would be 220 - 11 = 209 which is 104.5% of the borrowed amount. This is less than the 105% the protocol was aiming for. In fact, this figure of 104.5% goes down further to 103.5% when rewardPercentForCallingLiquidation = 5% and minimumCollateralRatioPercent = 115% which is much lower than protocol's buffer target. Not being aware of this risk can cause unexpected loss of funds.

A table outlining the real buffer upper limit values is provided below. Another table showing the actual desirable gap in values is also provided so that the buffer always is above 105%. <br>

Straightaway, it can be seen that the current default protocol values of 5% and 110% give a buffer of less than 105% and hence either the minimumCollateralRatioPercent needs to have a lower limit of 111 instead of 110, or there should be agreement to the fact that 103.5% is an acceptable remainingRatio figure under the current scheme of things.

Proof of Concept

All figures expressed as % of borrowed amount i.e. 104.5 means 100 was borrowed. <br>

Current Implementation:

<br>

Desired figures for maintaining an upper limit of >= 105% of borrowed amount:

Tools used

Manual review

Recommended Mitigation Steps

Assuming that the protocol wants to calculate an actual 105% remainingRatio, changes along these lines need to be made. Please note that you may have to additionally make sure rounding errors & precision loss do not creep in. These suggestions point towards a general direction:<br> Update the two functions:

	function changeRewardPercentForCallingLiquidation(bool increase) external onlyOwner
        {
        if (increase)
            {
			// 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 afterIncrease = rewardPercentForCallingLiquidation + 1;
+           uint256 remainingRatio = minimumCollateralRatioPercent - minimumCollateralRatioPercent * afterIncrease / 100;
-			uint256 remainingRatioAfterReward = minimumCollateralRatioPercent - rewardPercentForCallingLiquidation - 1;

-           if (remainingRatioAfterReward >= 105 && rewardPercentForCallingLiquidation < 10)
+           if (remainingRatio >= 105 && rewardPercentForCallingLiquidation < 10)
                rewardPercentForCallingLiquidation += 1;
            }
        else
            {
            if (rewardPercentForCallingLiquidation > 5)
                rewardPercentForCallingLiquidation -= 1;
            }

		emit RewardPercentForCallingLiquidationChanged(rewardPercentForCallingLiquidation);
        }

and

	function changeMinimumCollateralRatioPercent(bool increase) external onlyOwner
        {
        if (increase)
            {
            if (minimumCollateralRatioPercent < 120)
                minimumCollateralRatioPercent += 1;
            }
        else
            {
			// Don't decrease the minimumCollateralRatioPercent 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 afterDecrease = minimumCollateralRatioPercent - 1;
+           uint256 remainingRatio = afterDecrease - afterDecrease * rewardPercentForCallingLiquidation / 100;            
-			uint256 remainingRatioAfterReward = minimumCollateralRatioPercent - 1 - rewardPercentForCallingLiquidation;

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

		emit MinimumCollateralRatioPercentChanged(minimumCollateralRatioPercent);
        }

Also L39:

- 39:     uint256 public minimumCollateralRatioPercent = 110;
+ 39:     uint256 public minimumCollateralRatioPercent = 111;

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/ManagedWallet.sol#L59-L69

Vulnerability details

Impact

ManagedWallet.sol states that:

// A smart contract which provides two wallet addresses (a main and confirmation wallet) which can be changed using the following mechanism:
// 1. Main wallet can propose a new main wallet and confirmation wallet.
// 2. Confirmation wallet confirms or rejects.
// 3. There is a timelock of 30 days before the proposed mainWallet can confirm the change.

However, the current 30-day wait period can be bypassed. <br>

The expected order by the protocol is:

1. proposeWallets() is called by mainWallet.
2. To confirm the proposal, confirmationWallet sends at least 0.05 ether and causes the receive() function to trigger. This sets the activeTimelock to block.timestamp + TIMELOCK_DURATION i.e. 30 days into the future.
3. proposedMainWallet calls changeWallets() after 30 days and the new wallet addresses are set.

To bypass the 30-day limitation, the following flow can be used:

1. Even with no propsal for a change existing, confirmationWallet sends at least 0.05 ether and causes the receive() function to trigger. This sets the activeTimelock to block.timestamp + TIMELOCK_DURATION i.e. 30 days into the future.
2. Just as 30 days pass,
   • proposeWallets() is called by mainWallet
   • Immediately, with no delay whatsoever, proposedMainWallet calls changeWallets()
3. The new wallet addresses are successfully assigned.

Impact: Although the users believe that any changes to wallet address is going to have a timelock of 30 days as promised by the protocol, it really can be bypassed by the current admins/wallet addresses. This breaks the intended functionality implmentation.

Proof of Concept

Add the following inside 2024-01-salty/src/root_tests/ManagedWallet.t.sol and run with COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.sepolia.org/ --mt test_ManipulateActiveTimeLockBeforeProposingNewWallets:

    function test_ManipulateActiveTimeLockBeforeProposingNewWallets() public {
        // Set up the initial state with main and confirmation wallets
        address initialMainWallet = alice;
        address initialConfirmationWallet = address(0x2222);
        ManagedWallet managedWallet = new ManagedWallet(initialMainWallet, initialConfirmationWallet);

        // Set up the proposed main and confirmation wallets
        address newMainWallet = address(0x3333);
        address newConfirmationWallet = address(0x4444);

        // @audit : Even before any proposal exists, prank as the current confirmation wallet and send ether to start the TIMELOCK_DURATION 
        uint256 sentValue = 0.06 ether;
        vm.prank(initialConfirmationWallet);
        vm.deal(initialConfirmationWallet, sentValue);
        (bool success,) = address(managedWallet).call{value: sentValue}("");
        assertTrue(success, "Confirmation of wallet proposal failed");

        // Warp the blockchain time to the future beyond the active timelock period
        uint256 currentTime = block.timestamp;
        vm.warp(currentTime + TIMELOCK_DURATION);

        // Prank as the initial main wallet to propose the new wallets
        vm.startPrank(initialMainWallet);
        managedWallet.proposeWallets(newMainWallet, newConfirmationWallet);
        vm.stopPrank();

        // @audit : With NO DELAY, immediately prank as the new proposed main wallet which should now be allowed to call changeWallets
        vm.prank(newMainWallet);
        managedWallet.changeWallets();

        // Check that the mainWallet and confirmationWallet state variables are updated
        assertEq(managedWallet.mainWallet(), newMainWallet, "mainWallet was not updated correctly");
        assertEq(managedWallet.confirmationWallet(), newConfirmationWallet, "confirmationWallet was not updated correctly");

        // Check that the proposed wallets and activeTimelock have been reset
        assertEq(managedWallet.proposedMainWallet(), address(0), "proposedMainWallet was not reset");
        assertEq(managedWallet.proposedConfirmationWallet(), address(0), "proposedConfirmationWallet was not reset");
        assertEq(managedWallet.activeTimelock(), type(uint256).max, "activeTimelock was not reset to max uint256");
    }

Tools used

Foundry

Recommended Mitigation Steps

Inside receive() make sure an active proposal exists:

    receive() external payable
    	{
    	require( msg.sender == confirmationWallet, "Invalid sender" );
+       require( proposedMainWallet != address(0), "Cannot manipulate activeTimelock without active proposal" );

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

Assessed type

Access Control