c4-bot-1 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolsConfig.sol#L63-L74 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/RewardsEmitter.sol#L57-L77 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L182-L206

Vulnerability details

Summary

When a token is unwhitelisted, it's Pools are also unwhitelisted. These Pools may have accrued awards between the last time performUpkeep() was called and it's subsequent unwhitelisting. However, once a Pool is unwhitelisted, it is no longer eligible to receive Liquidity Rewards meaning users who provided liquidity to these pools will lose any rewards accrued from Arbitrage Profits.

Vulnerability Details

There may be arbitrage profits sitting in the Pools contract; a portion of which was generated by the Pool being unwhitelisted. There is no logic in PoolsConfig::unwhitelistPool() to account for those profits before unwhitelisting.

    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));
        }

After Pools have been unwhitelisted there is a check in RewardsEmitter::addSALTRewards() preventing any additional rewards being added to liquidityRewardsEmitter; so the rewards will remain in the saltRewards contract. There is a subsequent check in stakingRewards::addSALTRewards() which prevents additional rewards being added so the protocol cannot manually add rewards to rectify the issue.

    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" );

            uint256 amountToAdd = addedReward.amountToAdd;
            if ( amountToAdd != 0 )
                {
                // Update pendingRewards so the SALT can be distributed later
                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 );
        }

POC

Add the test function below to DAO.t.sol and run. The test shows Alice is able to claim due rewards from stakingRewards which had already accrued for an unwhitelisted Pool. It then shows that any new attempt to add rewards via liquidityRewardsEmitter::addSALTRewards or stakingRewards::addSALTRewards will revert.

```
function testClaimAllRewards() public {

    vm.startPrank(DEPLOYER);
    // Alice increases her share in pools[1] by 5 ether
    stakingRewards.externalIncreaseUserShare(alice, poolIDs[1], 5 ether, true);
    vm.stopPrank();

    // Add rewards to the pools
    AddedReward[] memory addedRewards = new AddedReward[](1);
    addedRewards[0] = AddedReward(poolIDs[1], 20 ether);
    stakingRewards.addSALTRewards(addedRewards);

    // Check Alice's pending rewards in both pools
    uint256 pendingRewardPool1 = stakingRewards.userRewardForPool(alice, poolIDs[1]);
    assertEq(pendingRewardPool1, 20 ether);

    // Check Alice's SALT balance before claiming
    uint256 aliceSaltBalanceBefore = salt.balanceOf(alice);

    // Whitelist is withdrawn for pool1
    vm.prank(address(dao));
    poolsConfig.unwhitelistPool( pools, token1, token2);

    // Alice claims all rewards from both pools
    bytes32[] memory claimPools = new bytes32[](1);
    claimPools[0] = poolIDs[1];
    vm.prank(alice);
    stakingRewards.claimAllRewards(claimPools);

    // Alice is able to claim any rewards which were added to stakingRewards prior to unwhitelisting
    uint256 aliceSaltBalanceAfter = salt.balanceOf(alice);
    assertEq(aliceSaltBalanceAfter, aliceSaltBalanceBefore + pendingRewardPool1);

    // New rewards cannot be added to unwhitlisted pool via liquidityRewardsEmitter
    AddedReward[] memory addedLiquidityRewards = new AddedReward[](1);
    addedLiquidityRewards[0] = AddedReward({poolID: poolIDs[1], amountToAdd: 100 ether});

    // In performUpkeep process rewards are first added to liquidityRewardsEmitter
    vm.prank(alice);
    vm.expectRevert("Invalid pool");
    liquidityRewardsEmitter.addSALTRewards(addedRewards);

    // Rewards move liquidityRewardsEmitter to stakingRewards
    vm.prank(alice);
    vm.expectRevert("Invalid pool");
    stakingRewards.addSALTRewards(addedRewards); 

}

```

Impact

Liquidity Providers to either Pool of an unwhitelisted token will lose any due rewards generated between the last execution of the upkeep process and the token being unwhitelisted.

Tools Used

Manual Review Foundry Testing

Recommendations

As part of the token unwhitelisting prcoess and before removal of the two PoolID from _whitelist storage variable; trigger upkeep steps to ensure that due rewards are updated so that they can be withdrawn by users.

Assessed type

Governance