Salty.IO - Topmark's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolStats.sol#L42 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolsConfig.sol#L68

Vulnerability details

Impact

When Pool Id are unwhitelisted in the PoolsConfig.sol contract Unclaimed Arbitrage Profit stored in the PoolStats contract would be completely lost

Proof of Concept

// Record that arbitrageProfit was generated and the a specific arbitrage path generated it (which is defined by the middle two tokens in WETH->arbToken2->arbToken3->WETH)
	function _updateProfitsFromArbitrage( IERC20 arbToken2, IERC20 arbToken3, uint256 arbitrageProfit ) internal
	{
	// Though three pools contributed to the arbitrage we can record just the middle one as we know the input and output token will be WETH
	bytes32 poolID = PoolUtils._poolID( arbToken2, arbToken3 );

>>>	_arbitrageProfits[poolID] += arbitrageProfit;
	}

The function above from the PoolStats.sol contract shows how arbitrageProfit is updated to the mapping of each PoolId

function unwhitelistPool( IPools pools, IERC20 tokenA, IERC20 tokenB ) external onlyOwner
	{
	bytes32 poolID = PoolUtils._poolID(tokenA,tokenB);

	_whitelist.remove(poolID);
>>>	delete underlyingPoolTokens[poolID];

	// Make sure that the cached arbitrage indicies in PoolStats are updated
	pools.updateArbitrageIndicies();

	emit PoolUnwhitelisted(address(tokenA), address(tokenB));
	}

While the function above from the PoolsConfig.sol contract shows how each poolId is unwhitelisted, The problem is that every factor was put into consideration such as removing it from _whitelist, deleting from underlyingPoolTokens mapping, and updating ArbitrageIndicies correctly but unclaimed _arbitrageProfits was not put into consideration before deleting the PoolIds, this would cause loss of fund as this array of PoolIds is still needed in other to claim profit.

Tools Used

Manual Review

Recommended Mitigation Steps

As adjusted in the code provided below protocol should check to ensure there is no unclaimed arbitrage profit left before removing Pool from whitelisted Pool ID

function unwhitelistPool( IPools pools, IERC20 tokenA, IERC20 tokenB ) external onlyOwner
	{
	bytes32 poolID = PoolUtils._poolID(tokenA,tokenB);

	_whitelist.remove(poolID);
	delete underlyingPoolTokens[poolID];
+++ if ( _arbitrageProfits[poolID] > 0 ){
+++     revert ( "Unclaimed arbitrage Profit" )
+++     }
+++     delete _arbitrageProfits[poolID];

		// Make sure that the cached arbitrage indicies in PoolStats are updated
		pools.updateArbitrageIndicies();

		emit PoolUnwhitelisted(address(tokenA), address(tokenB));
		}

Assessed type

Access Control

Report 1:

Missing Validation check for PoolIds in PoolStats contract.

When a pool Id is invalid it returns INVALID_POOL_ID which represents type(uint64).max, however this return value was not checked at L88-L91 in the updateArbitrageIndicies() function. This would allow invalid pool during Arbitrage and could escalate to serious problems, The return value should be checked as adjusted below. https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolStats.sol#L89-L91 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolStats.sol#L71

function _poolIndex( IERC20 tokenA, IERC20 tokenB, bytes32[] memory poolIDs ) internal pure returns (uint64 index)
	{
	bytes32 poolID = PoolUtils._poolID( tokenA, tokenB );

	for( uint256 i = 0; i < poolIDs.length; i++ )
		{
		if (poolID == poolIDs[i])
			return uint64(i);
		}

>>>	return INVALID_POOL_ID;
	}

function updateArbitrageIndicies() public
	{
	bytes32[] memory poolIDs = poolsConfig.whitelistedPools();

	for( uint256 i = 0; i < poolIDs.length; i++ )
	{
	bytes32 poolID = poolIDs[i];
	(IERC20 arbToken2, IERC20 arbToken3) = poolsConfig.underlyingTokenPair(poolID);

	// The middle two tokens can never be WETH in a valid arbitrage path as the path is WETH->arbToken2->arbToken3->WETH.
	if ( (arbToken2 != _weth) && (arbToken3 != _weth) )
				{
>>>	uint64 poolIndex1 = _poolIndex( _weth, arbToken2, poolIDs );
>>>	uint64 poolIndex2 = _poolIndex( arbToken2, arbToken3, poolIDs );
>>>	uint64 poolIndex3 = _poolIndex( arbToken3, _weth, poolIDs );
+++     require ( poolIndex1 != INVALID_POOL_ID && poolIndex2 != INVALID_POOL_ID && poolIndex3 != INVALID_POOL_ID , "INVALID_POOL_ID" );
				...
		}

Report 2:

Unused Variable

Possible Incomplete implementation in the performUpkeep(...) function of the RewardsEmitter.sol contracr. As noted in the code provided below, uint256 sum was declared and was continuously assigned a value of amountToAddForPool at every loop cycle, but sum was only assigned it was not used at all through the function implementation https://github.com/code-423n4/2024-01-salty/blob/main/src/rewards/RewardsEmitter.sol#L115-L128

function performUpkeep( uint256 timeSinceLastUpkeep ) external
   ...
>>> uint256 sum = 0;
      for( uint256 i = 0; i < poolIDs.length; i++ )
	{
	bytes32 poolID = poolIDs[i];
		// Each pool will send a percentage of the pending rewards based on the time elapsed since the last send
	uint256 amountToAddForPool = ( pendingRewards[poolID] * numeratorMult ) / denominatorMult;

	// Reduce the pending rewards so they are not sent again
	if ( amountToAddForPool != 0 )
		{
		pendingRewards[poolID] -= amountToAddForPool;

>>>		sum += amountToAddForPool;
		}

	// Specify the rewards that will be added for the specific pool
	addedRewards[i] = AddedReward( poolID, amountToAddForPool );
	}

// Add the rewards so that they can later be claimed by the users proportional to their share of the StakingRewards derived contract.
stakingRewards.addSALTRewards( addedRewards );
}

Report 3:

Typo Error

Typographical error in comment description of tokenWhitelistingBallotWithTheMostVotes() function in the Proposals.sol contract. The correct statement as corrected below should be "number of no votes" not "number no votes". https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L416

	// Returns the ballotID of the whitelisting ballot that currently has the most yes votes
---	// Requires that the quorum has been reached and that the number of yes votes is greater than the number no votes
+++	// Requires that the quorum has been reached and that the number of yes votes is greater than the number of no votes
	function tokenWhitelistingBallotWithTheMostVotes() external view returns (uint256)
{
uint256 quorum = requiredQuorumForBallotType( BallotType.WHITELIST_TOKEN);

uint256 bestID = 0;
uint256 mostYes = 0;
for( uint256 i = 0; i < _openBallotsForTokenWhitelisting.length(); i++ )
	{
	...
	}

return bestID;
}

Report 4:

Comment Misinterpretation

The comment in the code provided below in the Pools.sol contract is to determine the value WETH would worth after swap with SwapAmountIn and not just proportionate value, therefore should be adjusted as provided below. https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L301-L313

>>> // Determine the WETH equivalent of swapAmountIn of swapTokenIn
	function _determineSwapAmountInValueInETH(IERC20 swapTokenIn, uint256 swapAmountIn) internal view returns (uint256 swapAmountInValueInETH)
{
if ( address(swapTokenIn) == address(weth) )
	return swapAmountIn;

// All tokens within the DEX are paired with WETH (and WBTC) - so this pool will exist.
	(uint256 reservesWETH, uint256 reservesTokenIn) = getPoolReserves(weth, swapTokenIn);

if ( (reservesWETH < PoolUtils.DUST) || (reservesTokenIn < PoolUtils.DUST) )
	return 0; // can't arbitrage as there are not enough reserves to determine swapAmountInValueInETH

---    swapAmountInValueInETH = ( swapAmountIn * reservesWETH ) / reservesTokenIn;
+++    swapAmountInValueInETH = ( swapAmountIn * reservesWETH ) / ( reservesTokenIn + swapAmountIn );

if ( swapAmountInValueInETH <= PoolUtils.DUST )
	return 0;
}