Salty.IO - wangxx2026's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L115-L135 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/Liquidizer.sol#L101-L126 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/Liquidizer.sol#L132-L152 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L105-L108

Vulnerability details

Impact

The need to burn more usds for usds that have already been repaid can lead to the loss of unnecessary usds

Proof of Concept

With the repayUSDS function, we can see that when the user repays usds, the usds that the user repaid, are transferred directly to the usds contract to be used for burning. liquidizer.increaseBurnableUSDS increases the number of usds that can can be burned to be burned when calling Liquidizer._ possiblyBurnUSDS to burn the usds that need to be burned. https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L115-L135

     function repayUSDS( uint256 amountRepaid ) external nonReentrant
		{
...
		// Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
-->	usds.safeTransferFrom(msg.sender, address(usds), amountRepaid); // Transferring reimbursed usds from the user to the usds contract

		// Have USDS remember that the USDS should be burned
-->		liquidizer.incrementBurnableUSDS( amountRepaid ); // Record the number of usds that need to be burned, increase the value of Liquidizer.usdsThatShouldBeBurned
...
		}

We can see that when Upkeep.step1 is called, it will eventually call to Liquidizer._possiblyBurnUSDS to burn the usds that need to be burned.

At this point we find usdsBalance < usdsThatShouldBeBurned.This is because when the usds are repaid, the usds are transferred to the usds contract, not the Liquidizer contract.

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

	function _possiblyBurnUSDS() internal
		{
		// Check if there is USDS to burn
		if ( usdsThatShouldBeBurned == 0 )
			return;

		uint256 usdsBalance = usds.balanceOf(address(this));
		if ( usdsBalance >= usdsThatShouldBeBurned )
			{
			// Burn only up to usdsThatShouldBeBurned.
			// Leftover USDS will be kept in this contract in case it needs to be burned later.
			_burnUSDS( usdsThatShouldBeBurned );
    		usdsThatShouldBeBurned = 0;
			}
		else
			{
			// The entire usdsBalance will be burned - but there will still be an outstanding balance to burn later
			_burnUSDS( usdsBalance );
			usdsThatShouldBeBurned -= usdsBalance;

			// As there is a shortfall in the amount of USDS that can be burned, liquidate some Protocol Owned Liquidity and
			// send the underlying tokens here to be swapped to USDS
			dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW);
			dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);
			}
		}

If we assume that only one user repaid 1000usds, at this time usdsBalance=0, it will execute else. eventually the method usds.burnTokensInContract will be called. 1000usds is burned. But at this point usdsThatShouldBeBurned is still 1000. then the salt, dai, and usds assets are transferred from the dao to be used for the burning of these 1000usds

Tools Used

Manual Review

Recommended Mitigation Steps

When the usds are repaid . Put

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

usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

Modify to:

usds.safeTransferFrom(msg.sender, address(liquidizer), amountRepaid);

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Malicious users can redefine the ratio of pool

Proof of Concept

In the removeLiquidity function, we can see that there is a restriction to prevent the user from setting the liquidity to 0. However, only reserves.reserve0>PoolUtils.DUST is restricted, not reserves.reserve1

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

	function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
		{
...
		// 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"); // Limit the balance after removal but only reserves.reserve0

...
		}

A malicious user can first deposit a certain amount of liquidity and then trade through the swap method to make reserve1 very close to PoolUtils.DUST, and then remove the liquidity to make reserves.reserve1=0. Then add liquidity to redefine the ratio of the currency pair. https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L90-L135

	function _addLiquidity( bytes32 poolID, uint256 maxAmount0, uint256 maxAmount1, uint256 totalLiquidity ) internal returns(uint256 addedAmount0, uint256 addedAmount1, uint256 addedLiquidity)
		{
		PoolReserves storage reserves = _poolReserves[poolID];
		uint256 reserve0 = reserves.reserve0;
		uint256 reserve1 = reserves.reserve1;

		// If either reserve is zero then consider the pool to be empty and that the added liquidity will become the initial token ratio
-->		if ( ( reserve0 == 0 ) || ( reserve1 == 0 ) ) // Redefine the ratio of the currency pair by making reserve1 = 0
			{
			// Update the reserves
			reserves.reserve0 += uint128(maxAmount0);
			reserves.reserve1 += uint128(maxAmount1);

			// Default liquidity will be the addition of both maxAmounts in case one of them is much smaller (has smaller decimals)
			return ( maxAmount0, maxAmount1, (maxAmount0 + maxAmount1) );
			}

...
		}

For example, define the original ratio of 1:1000 as 10:1. arbitrage is achieved by taking the reserve0 deposited by the previous transaction.

Tools Used

Manual Review

Recommended Mitigation Steps

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

        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

Modify the code to

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/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L59-L69 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L73-L89

Vulnerability details

Impact

confirmationWallet can confirm changes in advance

Proof of Concept

With the receive function, we can see that confirmationWallet can be called at any time, and as long as msg.value >= .05 ether, activeTimelock will be modified to the current time + lock time. https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L59-L69

    receive() external payable
    	{
    	require( msg.sender == confirmationWallet, "Invalid sender" );

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

This can violate the predefined flow of the contract. For example, the confirmationWallet calls the receive method 30 days (beyond the lockout period) ahead of time when there is no proposedMainWallet to modify. Then the process of modifying the wallet becomes, mainWallet calls proposeWallets and then calls changeWallets to modify the completion. The intermediate confirmation and waiting periods are omitted.

Tools Used

Manual Review

Recommended Mitigation Steps

    receive() external payable
    	{
    	require( msg.sender == confirmationWallet, "Invalid sender" );

		// 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 )
-->		require( proposedMainWallet != address(0), "Without proposedMainWallet modification requires confirmation." ); // Add restriction that only modified proposedMainWallet can be confirmed.
    		activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
    	else
			activeTimelock = type(uint256).max; // effectively never
        }

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L194-L201 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L134-L140 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L104-L127 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L47-L55 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L61-L72 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L77-L99

Vulnerability details

Impact

Can cause arbitrage rewards to be lost

Proof of Concept

When allocating rewards in Upkeep.step7, the PoolStats._calculateArbitrageProfits method is called to calculate the rewards. https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L104-L127

	function _calculateArbitrageProfits( bytes32[] memory poolIDs, uint256[] memory _calculatedProfits ) internal view
		{
		for( uint256 i = 0; i < poolIDs.length; i++ )
			{
			// references poolID(arbToken2, arbToken3) which defines the arbitage path of WETH->arbToken2->arbToken3->WETH
			bytes32 poolID = poolIDs[i];

			// Split the arbitrage profit between all the pools that contributed to generating the arbitrage for the referenced pool.
			uint256 arbitrageProfit = _arbitrageProfits[poolID] / 3;
			if ( arbitrageProfit > 0 )
				{
			     ArbitrageIndicies memory indicies = _arbitrageIndicies[poolID];

		-->		if ( indicies.index1 != INVALID_POOL_ID ) // Rewards will only be distributed if the pool is whitelisted
					_calculatedProfits[indicies.index1] += arbitrageProfit;

				if ( indicies.index2 != INVALID_POOL_ID )
					_calculatedProfits[indicies.index2] += arbitrageProfit;

				if ( indicies.index3 != INVALID_POOL_ID )
					_calculatedProfits[indicies.index3] += arbitrageProfit;
				}
			}
		}

We can see that the arbitrage rewards are distributed equally to the 3 pools that contributed to the arbitrage. After the distribution is done, PoolStats.clearProfitsForPools is called to zero out the arbitrage.

The problem is that only the whitelisted pools will be assigned rewards, and the rewards of the pools that are not whitelisted will be lost.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the _calculateArbitrageProfits function to:

	function _calculateArbitrageProfits( bytes32[] memory poolIDs, uint256[] memory _calculatedProfits ) internal view
		{
		for( uint256 i = 0; i < poolIDs.length; i++ )
			{
			// references poolID(arbToken2, arbToken3) which defines the arbitage path of WETH->arbToken2->arbToken3->WETH
			bytes32 poolID = poolIDs[i];

			// Split the arbitrage profit between all the pools that contributed to generating the arbitrage for the referenced pool.
			uint256 arbitrageProfit = _arbitrageProfits[poolID];
			if ( arbitrageProfit > 0 )
				{
					ArbitrageIndicies memory indicies = _arbitrageIndicies[poolID];
					uint256 validIndicies;
					if ( indicies.index1 != INVALID_POOL_ID )
					++validIndicies;

					if ( indicies.index2 != INVALID_POOL_ID )
					++validIndicies;
						
					if ( indicies.index3 != INVALID_POOL_ID )
					++validIndicies;
					
					if (validIndicies > 0) 
					{
						arbitrageProfitToSend = arbitrageProfit/validIndicies;
						if ( indicies.index1 != INVALID_POOL_ID )
							_calculatedProfits[indicies.index1] += arbitrageProfitToSend;

						if ( indicies.index2 != INVALID_POOL_ID )
							_calculatedProfits[indicies.index2] += arbitrageProfitToSend;

						if ( indicies.index3 != INVALID_POOL_ID )
							_calculatedProfits[indicies.index3] += arbitrageProfitToSend;
					} else {
						// todo If none of the pools are whitelisted, send the rewards to the caller or distribute them equally to the other pools.
					}
				}
			}
		}

Assessed type

Math