Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L160

Vulnerability details

Impact

Pending amount of SALT frozen in liquidityRewardsEmitter for unwhitelisted token cannot be used.

Proof of Concept

Pending amount of SALT-token which liquidityRewardsEmitter didn't have distributed yet remains frozen if unwhitelist token.

SaltRewards.sol#_sendLiquidityRewards function which distributes rewards for whitelisted pool when upkeep is as follows.

    function _sendLiquidityRewards( uint256 liquidityRewardsAmount, uint256 directRewardsForSaltUSDS, bytes32[] memory poolIDs, uint256[] memory profitsForPools, uint256 totalProfits ) internal
        {
        require( poolIDs.length == profitsForPools.length, "Incompatible array lengths" );

        // Send SALT rewards (with an amount of pendingLiquidityRewards) proportional to the profits generated by each pool
        AddedReward[] memory addedRewards = new AddedReward[]( poolIDs.length );
        for( uint256 i = 0; i < addedRewards.length; i++ )
            {
            bytes32 poolID = poolIDs[i];
            uint256 rewardsForPool = ( liquidityRewardsAmount * profitsForPools[i] ) / totalProfits;

            // The SALT/USDS pool is entitled to additional rewards - as specified by RewardsConfig.percentRewardsSaltUSDS
            if ( poolID == saltUSDSPoolID )
                rewardsForPool += directRewardsForSaltUSDS;

            addedRewards[i] = AddedReward( poolID, rewardsForPool );
            }

        // Send the SALT rewards to the LiquidityRewardsEmitter
77@     liquidityRewardsEmitter.addSALTRewards( addedRewards );
        }

RewardsEmitter.sol#addSALTRewards function called on L77 is as follows.

    function addSALTRewards( AddedReward[] calldata addedRewards ) external nonReentrant
        {
        uint256 sum = 0;
        for( uint256 i = 0; i < addedRewards.length; i++ )
            {
            AddedReward memory addedReward = addedRewards[i];
            require( poolsConfig.isWhitelisted( addedReward.poolID ), "Invalid pool" );

65          uint256 amountToAdd = addedReward.amountToAdd;
            if ( amountToAdd != 0 )
                {
                // Update pendingRewards so the SALT can be distributed later
69              pendingRewards[ addedReward.poolID ] += amountToAdd;
                sum += amountToAdd;
                }
            }

        // Transfer the SALT from the sender for all of the specified rewards
        if ( sum > 0 )
            salt.safeTransferFrom( msg.sender, address(this), sum );
        }

It checks if each pool is whitelisted on L65 and increases pending reward on L69.

On the other hand, RewardsEmitter.sol#performUpkeep function which distributes rewards is as follows.

    function performUpkeep( uint256 timeSinceLastUpkeep ) external
        {
        require( msg.sender == address(exchangeConfig.upkeep()), "RewardsEmitter.performUpkeep is only callable from the Upkeep contract" );

        if ( timeSinceLastUpkeep == 0 )
            return;

        bytes32[] memory poolIDs;

         if ( isForCollateralAndLiquidity )
            {
            // For the liquidityRewardsEmitter, all pools can receive rewards
94          poolIDs = poolsConfig.whitelistedPools();
            }
         else
            {
            // The stakingRewardsEmitter only distributes rewards to those that have staked SALT
            poolIDs = new bytes32[](1);
            poolIDs[0] = PoolUtils.STAKED_SALT;
            }

        // Cap the timeSinceLastUpkeep at one day (if for some reason it has been longer).
        // This will cap the emitted rewards at a default of 1% in this transaction.
        if ( timeSinceLastUpkeep >= MAX_TIME_SINCE_LAST_UPKEEP )
            timeSinceLastUpkeep = MAX_TIME_SINCE_LAST_UPKEEP;

        // These are the AddedRewards that will be sent to the specified StakingRewards contract
        AddedReward[] memory addedRewards = new AddedReward[]( poolIDs.length );

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

        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
121         uint256 amountToAddForPool = ( pendingRewards[poolID] * numeratorMult ) / denominatorMult;

            // Reduce the pending rewards so they are not sent again
            if ( amountToAddForPool != 0 )
                {
126             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.
136     stakingRewards.addSALTRewards( addedRewards );
        }

As we can see above, on L121 partial amount of pending reward is calculated and on L138 they are distributed.

And DAO.sol#_executeApproval function which unwhitelists pool is as follows.

    function _executeApproval( Ballot memory ballot ) internal
        {
        if ( ballot.ballotType == BallotType.UNWHITELIST_TOKEN )
            {
            // All tokens are paired with both WBTC and WETH so unwhitelist those pools
            poolsConfig.unwhitelistPool( pools, IERC20(ballot.address1), exchangeConfig.wbtc() );
            poolsConfig.unwhitelistPool( pools, IERC20(ballot.address1), exchangeConfig.weth() );

            emit UnwhitelistToken(IERC20(ballot.address1));
            }

        ...
        }

As we can see, there isn't any process which collects pending rewards frozen to liquidityRewardsEmitter when unwhitelisting pool.

Tools Used

Manual Review

Recommended Mitigation Steps

Mitigating steps for this problem are as follows.

Add RewardsEmitter.sol#collectPendingRewards function as follows.

function collectPendingRewards(bytes32 poolID) external{
    require(msg.sender == exchangeConfig.dao(), "only callable from dao.");
    uint256 amount = pendingRewards[poolID];
    pendingRewards[poolID] = 0;
    salt.safeTransfer(msg.sender, amount);
}

DAO.sol#_executeApproval function has to be modified as follows.


function _executeApproval( Ballot memory ballot ) internal
    {
    if ( ballot.ballotType == BallotType.UNWHITELIST_TOKEN )
        {
        // All tokens are paired with both WBTC and WETH so unwhitelist those pools
        poolsConfig.unwhitelistPool( pools, IERC20(ballot.address1), exchangeConfig.wbtc() );
        poolsConfig.unwhitelistPool( pools, IERC20(ballot.address1), exchangeConfig.weth() );

bytes32 pool1 = PoolUtils._poolID( IERC20(ballot.address1), exchangeConfig.wbtc() );
bytes32 pool2 = PoolUtils._poolID( IERC20(ballot.address1), exchangeConfig.weth() );
liquidityRewardsEmitter.collectPendingRewards(pool1);

liquidityRewardsEmitter.collectPendingRewards(pool2);

    emit UnwhitelistToken(IERC20(ballot.address1));
    }
    ...
}

Assessed type

Other

code-423n4 / 2024-01-salty-findings

Pending amount of SALT-token is not collected from liquidityRewardsEmitter when unwhitelisting pool. #864