Salt Rewards - Rewards related to Arbitrage profits for pools can be lost

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

Arbitrage profits are distributed to pools that played a part in generating them. This is distributed via calling the performUpkeep function with upkeep.sol.

The process of upkeep is multi step, and any failure in any step does not disable next steps to trigger because each step is wrapped in a try catch. lets delve into steps 5 and 7.

    // 5. Convert remaining WETH to SALT and sends it to SaltRewards.
function step5() public onlySameContract
    {
    uint256 wethBalance = weth.balanceOf( address(this) );
    if ( wethBalance == 0 )
        return;

    // Convert remaining WETH to SALT and send it to SaltRewards
    // @audit arbitrage profits sent to saltrewards here, pools contract
    // has allowance to withdrawm unlimited WETH from the upkeep contract
    // but this swap operation can fail, if arbitrage profits are too large
    // and not enough reserves of salt are there
    uint256 amountSALT = pools.depositSwapWithdraw( weth, salt, wethBalance, 0, block.timestamp );
    salt.safeTransfer(address(saltRewards), amountSALT);
    }

step 5 converts the arbitrage profits from WETH to Salt. Next, after which they are distributed to SaltRewards contract. If the operation to swap WETH to Salt fails due to reserves going below dust for example, then no Salt will be transferred to the salt rewards contract. this will impact step 7.

    // 7. Distribute SALT from SaltRewards to the stakingRewardsEmitter and liquidityRewardsEmitter.
function step7() public onlySameContract
    {
    // @audit line below can return a list with arbitrage profits assigned for a pool
    uint256[] memory profitsForPools = pools.profitsForWhitelistedPools();

    bytes32[] memory poolIDs = poolsConfig.whitelistedPools();
    // @audit if more than 1 week passed, less rewards passed to the emitters in step 8.
    saltRewards.performUpkeep(poolIDs, profitsForPools );
    // @audit can arbitrage profits be cleared without distributing?
    pools.clearProfitsForPools();
    }

Step 6 will distribute the salt emissions to the saltrewards contract. so the saltrewards contract will have only salt related to emissions but not arbitrage profits.

Step 7 will distribute all salt rewards(including those arbitrage profits in Salt to all the pools) via calling performUpkeep function in salt rewards contrat, but this assumes the SaltRewards contract will have a salt balance containing the arbitrage profits (from step 5), which might not be the case.

        uint256 saltRewardsToDistribute = salt.balanceOf(address(this));
    if ( saltRewardsToDistribute == 0 ){ // @audit function simply returns, this could be problematic
        return;
    }

    // Determine the total profits so we can calculate proportional share for the liquidity rewards
    uint256 totalProfits = 0;
    for( uint256 i = 0; i < poolIDs.length; i++ ) {
        totalProfits += profitsForPools[i];
    }

    // Make sure that there are some profits to determine the proportional liquidity rewards.
    // Otherwise just handle the SALT balance later so it can be divided between stakingRewardsEmitter and liquidityRewardsEmitter without further accounting.
    if ( totalProfits == 0 ) {
        return;
    }

    // Determine how much of the SALT rewards will be directly awarded to the SALT/USDS pool.
    // This is because SALT/USDS is important, but not included in other arbitrage trades - which would normally yield additional rewards for the pool by being part of arbitrage swaps.
    uint256 directRewardsForSaltUSDS = ( saltRewardsToDistribute * rewardsConfig.percentRewardsSaltUSDS() ) / 100;
    uint256 remainingRewards = saltRewardsToDistribute - directRewardsForSaltUSDS;

    // Divide up the remaining rewards between SALT stakers and liquidity providers
    uint256 stakingRewardsAmount = ( remainingRewards * rewardsConfig.stakingRewardsPercent() ) / 100;
    uint256 liquidityRewardsAmount = remainingRewards - stakingRewardsAmount;
    _sendStakingRewards(stakingRewardsAmount);
    _sendLiquidityRewards(liquidityRewardsAmount, directRewardsForSaltUSDS, poolIDs, profitsForPools, totalProfits); // @audit salt rewards for liquidity will not include amounts related to pool arbitrate profits

As you can see above, the performupkeep checks the contracts salt balance and based off that the liquidity rewards amounts are determined. but given that actual balance does not include those profits, these figures will be inaccurate.

It would then return back to step7 and 'Clear' the profits assigned to each pool via calling pools.clearProfitsForPools().

This essentially means that profits assigned to pools are cleared from storage without actually being distributed fairly as Salt rewards to pool liquidity providers.

The likelihood of this bug occuring is low to medium, because it is dependant on failure in the WETH to Salt swap in step 5 which might occur if the pool was maliciously targeted or in extreme market conditions and volatility. However the impact is medium to high since pool liquidity providers will most certainly lose out on arbitrage profits(dependant the frequency of calling upkeep and trading dynamics, this could potentially be a large arbitrage profit), so given the potential for lost rewards, a rating of atleast high here is appropriate.

Proof of Concept

The POC below succesfully simulates this situation, by making sure the attempt to swap WETH to Salt in step 5 fails. this is done by manipulating pool reserve amounts. There are two versions, one with call to step 6 and one without.The one without the call to step6 more clearly displays the 'loss' in rewards.

function testSuccessStep5NoStep6() public
    {
    _setupLiquidity();
    vm.warp( block.timestamp + 1 days );
    _generateArbitrageProfits(true);

    // stakingRewardsEmitter and liquidityRewardsEmitter have initial bootstrapping rewards
    bytes32[] memory poolIDsB = new bytes32[](1);
    poolIDsB[0] = PoolUtils._poolID(salt, usds);
    uint256 initialRewardsB = liquidityRewardsEmitter.pendingRewardsForPools(poolIDsB)[0];

    bool errorRaised = false;

    //Mimic depositing arbitrage profits.
    vm.prank(DEPLOYER);
    weth.transfer(address(dao), 700 ether);

    vm.startPrank(address(dao));
    weth.approve(address(pools), 700 ether );
    pools.deposit( weth, 700 ether);
    vm.stopPrank();

    assertEq( salt.balanceOf(address(saltRewards)), 0 );

    vm.startPrank(address(upkeep));
    ITestUpkeep(address(upkeep)).step2(alice);
    ITestUpkeep(address(upkeep)).step3();
    ITestUpkeep(address(upkeep)).step4();
    try ITestUpkeep(address(upkeep)).step5() {}
    catch (bytes memory error) {errorRaised = true; }
    assertEq( errorRaised, true ); // proves that the swap from WETH to Salt in step 5 failed

    // No SALT was sent to SaltRewards, because swap from WETH to SALT failed due to low reserves.
    // which means arbitrage profits reward wont be sent to pools
    assertEq( salt.balanceOf(address(saltRewards)), 0);

    // Check that the rewards were recorded in storage
    bool poolsHaveProfit = false;
    uint256[] memory profitsForPools = IPoolStats(address(pools)).profitsForWhitelistedPools();
    for( uint256 i = 0; i < profitsForPools.length; i++ ) {
        console.log("pool profit for pool before:", i);
        console.log(profitsForPools[i]);
        if (profitsForPools[i] > 0){
            poolsHaveProfit = true;
        }

    }
    assertEq( poolsHaveProfit, true );

    ITestUpkeep(address(upkeep)).step7();
    bool poolsDontHaveProfit = false;
    // Check that the rewards for pools no longer recorded in storage
    uint256[] memory profitsForPools2 = IPoolStats(address(pools)).profitsForWhitelistedPools();
    for( uint256 i = 0; i < profitsForPools2.length; i++ ) {
        console.log("pool profit for pool:", i);
        console.log(profitsForPools2[i]);
        if (profitsForPools2[i] > 0){
            poolsDontHaveProfit = true;
        }
    }
    // test proves that in the case a pool doesnt have enough reserve to swap the arbitrage profits to SALT, the upkeep sequence
    // can lead to arbitrage profits being cleared from storage, effectively meaning they can not be distributed
    // to anyone, hence lost funds. ie  the pools that have taken part in generating recent arbitrage profits so that those pools 
    // will be unable to can receive proportional pending rewards on the next call to performUpkeep(), because their record has been cleared from storage.
    assertEq( poolsDontHaveProfit, false );

    bytes32[] memory poolIDs = new bytes32[](4);
    poolIDs[0] = PoolUtils._poolID(salt,weth);
    poolIDs[1] = PoolUtils._poolID(salt,wbtc);
    poolIDs[2] = PoolUtils._poolID(wbtc,weth);
    poolIDs[3] = PoolUtils._poolID(salt,usds);

    // Check that rewards were not sentto the three pools involved in generating the test arbitrage
    assertEq( liquidityRewardsEmitter.pendingRewardsForPools(poolIDs)[0], initialRewardsB );
    assertEq( liquidityRewardsEmitter.pendingRewardsForPools(poolIDs)[1], initialRewardsB );
    assertEq( liquidityRewardsEmitter.pendingRewardsForPools(poolIDs)[2], initialRewardsB );

    vm.stopPrank();
    }

    function testSuccessStep5Withstep6() public
    {
    _setupLiquidity();
    vm.warp( block.timestamp + 1 days );
    _generateArbitrageProfits(true);

    // stakingRewardsEmitter and liquidityRewardsEmitter have initial bootstrapping rewards
    bytes32[] memory poolIDsB = new bytes32[](1);
    poolIDsB[0] = PoolUtils._poolID(salt, usds);
    uint256 initialRewardsB = liquidityRewardsEmitter.pendingRewardsForPools(poolIDsB)[0];

    bool errorRaised = false;

    //Mimic depositing arbitrage profits.
    vm.prank(DEPLOYER);
    weth.transfer(address(dao), 700 ether);

    vm.startPrank(address(dao));
    weth.approve(address(pools), 700 ether );
    pools.deposit( weth, 700 ether);
    vm.stopPrank();

    assertEq( salt.balanceOf(address(saltRewards)), 0 );

    vm.startPrank(address(upkeep));
    ITestUpkeep(address(upkeep)).step2(alice);
    ITestUpkeep(address(upkeep)).step3();
    ITestUpkeep(address(upkeep)).step4();
    try ITestUpkeep(address(upkeep)).step5() {}
    catch (bytes memory error) {errorRaised = true; }
    assertEq( errorRaised, true ); // proves that the swap from WETH to Salt in step 5 failed

    // No SALT was sent to SaltRewards, because swap from WETH to SALT failed due to low reserves.
    // which means salt rewards related to arbitrage profits reward wont be sent to pools
    assertEq( salt.balanceOf(address(saltRewards)), 0);

    // Check that the rewards were recorded in storage
    bool poolsHaveProfit = false;
    uint256[] memory profitsForPools = IPoolStats(address(pools)).profitsForWhitelistedPools();
    for( uint256 i = 0; i < profitsForPools.length; i++ ) {
        console.log("pool profit for pool before:", i);
        console.log(profitsForPools[i]);
        if (profitsForPools[i] > 0){
            poolsHaveProfit = true;
        }

    }
    assertEq( poolsHaveProfit, true );

    ITestUpkeep(address(upkeep)).step6();
    ITestUpkeep(address(upkeep)).step7();
    bool poolsDontHaveProfit = false;
    // Check that the rewards for pools no longer recorded in storage
    uint256[] memory profitsForPools2 = IPoolStats(address(pools)).profitsForWhitelistedPools();
    for( uint256 i = 0; i < profitsForPools2.length; i++ ) {
        console.log("pool profit for pool:", i);
        console.log(profitsForPools2[i]);
        if (profitsForPools2[i] > 0){
            poolsDontHaveProfit = true;
        }
    }
    // test proves that in the case a pool doesnt have enough reserve to swap the arbitrage profits to SALT, the upkeep sequence
    // can lead to arbitrage profits being cleared from storage, effectively meaning they can not be distributed
    // to anyone, hence lost funds. ie  the pools that have taken part in generating recent arbitrage profits so that those pools 
    // will be unable to can receive proportional pending rewards on the next call to performUpkeep(), because their record has been cleared from storage.
    assertEq( poolsDontHaveProfit, false );

    vm.stopPrank();
    }

 function _setupLiquidity() internal
    {
    vm.prank(address(collateralAndLiquidity));
    usds.mintTo(DEPLOYER, 100000 ether );

    vm.prank(address(teamVestingWallet));
    salt.transfer(DEPLOYER, 100000 ether );

    vm.startPrank(DEPLOYER);
    weth.approve( address(collateralAndLiquidity), 300000 ether);
    usds.approve( address(collateralAndLiquidity), 100000 ether);
    dai.approve( address(collateralAndLiquidity), 100000 ether);
    salt.approve( address(collateralAndLiquidity), 100000 ether);

    collateralAndLiquidity.depositLiquidityAndIncreaseShare(weth, usds, 100000 ether, 100000 ether, 0, block.timestamp, false);
    collateralAndLiquidity.depositLiquidityAndIncreaseShare(weth, dai, 100000 ether, 100000 ether, 0, block.timestamp, false);
    collateralAndLiquidity.depositLiquidityAndIncreaseShare(weth, salt, 100000000000 wei, 100000000000 wei, 0, block.timestamp, false);

    vm.stopPrank();
    }

 function _generateArbitrageProfits( bool despositSaltUSDS ) internal
    {
    /// Pull some SALT from the daoVestingWallet
    vm.prank(address(daoVestingWallet));
    salt.transfer(DEPLOYER, 100000 ether);

    // Mint some USDS
    vm.prank(address(collateralAndLiquidity));
    usds.mintTo(DEPLOYER, 1000 ether);

    vm.startPrank(DEPLOYER);
    salt.approve(address(collateralAndLiquidity), type(uint256).max);
    wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
    weth.approve(address(collateralAndLiquidity), type(uint256).max);
    wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
    weth.approve(address(collateralAndLiquidity), type(uint256).max);

    if ( despositSaltUSDS )
        //collateralAndLiquidity.depositLiquidityAndIncreaseShare( salt, weth, 1000 ether, 1000 ether, 0, block.timestamp, false );
        collateralAndLiquidity.depositLiquidityAndIncreaseShare( salt, weth, 100000000000 wei, 100000000000 wei, 0, block.timestamp, false );

    collateralAndLiquidity.depositLiquidityAndIncreaseShare( wbtc, salt, 1000 * 10**8, 1000 ether, 0, block.timestamp, false );
    collateralAndLiquidity.depositCollateralAndIncreaseShare( 1000 * 10**8, 1000 ether, 0, block.timestamp, false );

    salt.approve(address(pools), type(uint256).max);
    wbtc.approve(address(pools), type(uint256).max);
    weth.approve(address(pools), type(uint256).max);
    vm.stopPrank();

    // Place some sample trades to create arbitrage profits
    _swapToGenerateProfits();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

given the interdependencies between step 5 and step 7, a failure in step5 due to a failed swap implies that step7 should also not proceed because its calculations are dependent on the success of step5. appropriate logic should be added to handle this.

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

It is acceptable for step 7 to not distribute rewards if step 5 has not functioned correctly. Assuming step 5 functions correctly later, then step 7 will function correctly later as well.

[Picodes]

It seems to me that rewards are just delayed here as the step 5 uses the contract's balance so there is no issue

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[genesiscrew]

There is no guarantee that the rewards that were delayed will be later distributed fairly to the pools that generated them. This is due to the fact that in the case of distribution failure, profits assigned to pools are cleared from storage as is shown in the POC.

Imagine pool A accumulated most of the rewards in interval 1, reward distribution fails, in interval two pool B accumulated most of the rewards, they will be rewarded more than they should be because the balance will include rewards from interval 1 and interval 2.

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

the line above shows how rewards are distributed for each pool. in this formula profitsForPools[i] and totalProfits will not factor in delayed rewards. while liquidityRewardsAmount will because its based off contract balance. so this means pool B in interval 2 will be effectively earning more rewards than it should, hence lost rewards for pool A.

[Picodes]

Thanks @genesiscrew. On second read it seems you are right. We could imagine a scenario where an attacker forces step 5 to fail to clear the storage and then distribute profits to a pool he is in. But the scenario wouldn't be simple because you need to take into account the fact that automatic arbitrages increases the cost of manipulating pool reserves.

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report