c4-bot-4 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L37 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L134 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolStats.sol#L47

Vulnerability details

Impact

The Upkeep::performUpkeep function, in step 2, withdraws profits generated from arbitrage within the pools. These rewards are then distributed among the involved pools. The system accumulates arbitrage profits for each pool in the PoolStats::_updateProfitsFromArbitrage function. The execution process is as follows:

Upkeep::perfomUpkeep is called, and it invokes dao::withdrawArbitrageProfits.
The dao::withdrawArbitrageProfits function retrieves the arbitrage generated for the DAO in wETH.
Subsequently, wETH is distributed among the POLs, and the remaining wETH is sent to the SaltRewards and then it distributes to the pools that contributed to arbitrage through the SaltRewards::performUpkeep function.

The issue arises when a pool that has generated arbitrage profits is deactivated using the poolsConfig::unwhitelistPool function. This causes the arbitrage-generated wETH from the unwhitelisted pool to be sent to SaltRewards but without delivering those arbitrage profits to the pool since the pool is no longer active. Additionally, the PoolStats::_arbitrageProfits variable of the deactivated pool is not cleared, causing the system to continue counting arbitrage profits that have already been sent to SaltRewards.

Considering the above, let's examine the following scenario:

Suppose poolAB generates 5 wETH, and poolBC generates 5 wETH as arbitrage profit.
poolBC is unwhitelisted.
Upkeep::perfomUpkeep is called, invoking dao::withdrawArbitrageProfits, obtaining the total 10e18 wETH tokens, including the profits from the deactivated pool (poolBC).
The 10e18 wETH tokens are distributed among the POLs, and the remainder is sent to SaltRewards to the whitelisted pools. In SaltRewards::performUpkeep, it is distributed only to the whitelisted poolAB, leaving the rest wETH in the SaltRewards contract.

Therefore, in the given example, the PoolStats::_arbitrageProfits variable of the deactivated pool (poolBC) remains unchanged. However, these profits have already been distributed among the POLs and SaltRewards to the whitelisted pools. Additionally, if the deactivated pool is reactivated, the PoolStats::_arbitrageProfits variable will have a non-zero value, causing it to count arbitrage profits that were previously delivered to SaltRewards.

This demonstrates that even though those profits from the unwhitelisted pool have already been distributed improperly to SaltRewards, the system will still take into account that the inactive pool has arbitrage profits, which can be detrimental if that pool becomes active again.

Proof of Concept

A test scenario was created to illustrate the issue. In the test, the poolIDBC is assigned 30e18 as profits. Subsequently, poolIDBC is deactivated, and the PoolStats::clearProfitsForPools function does not clear PoolStats::_arbitrageProfits for poolBC, leaving it unchanged at 30e18.

Setup test. Whitelist pools, deposit some profits from arbitrage, calculate and verify the expected profits per pool .
Unwhitelist the tokenB/tokenC pool.
Simulate the profits are sent to the pools. arbitrageProfits for poolIDAB is empty because those rewards were sent to the poolIDAB.
poolIDBC still has arbitrageProfits=30e18 which is incorrectly because dao::withdrawArbitrageProfits will extract all wETH arbitrage profits.

// Filename: src/pools/tests/PoolStats.t.sol:TestPoolStats
// $ forge test --rpc-url https://yourrpc.com --match-test="testProfitsForPoolsThenUnWhitelistAPool" -vvv
//
    function testProfitsForPoolsThenUnWhitelistAPool() public {
        //
        // 1. Setup test. Whitelist pools, deposit some profits from arbitrage, calculate and verify the expected profits per pool 
        // Whitelist pools
        vm.startPrank(address(deployment.dao()));
        deployment.poolsConfig().whitelistPool(deployment.pools(), tokenA, tokenB);
        deployment.poolsConfig().whitelistPool(deployment.pools(), tokenB, tokenC);
        vm.stopPrank();
        // Update arbitrage indicies
        this.updateArbitrageIndicies();
        // Set arbitrage profits
        this.updateProfitsFromArbitrage(tokenA, tokenB, 20 ether);
        this.updateProfitsFromArbitrage(tokenB, tokenC, 30 ether);
        // Get profits for whitelisted pools
        uint256[] memory profits = this.profitsForWhitelistedPools();
        // Fetch the pool IDs
        bytes32 poolIDAB = PoolUtils._poolID(tokenA, tokenB);
        bytes32 poolIDBC = PoolUtils._poolID(tokenB, tokenC);
        // Fetch the pool indexes
        bytes32[] memory whitelistedPools = poolsConfig.whitelistedPools();
        uint64 indexAB = _poolIndex(tokenA, tokenB, whitelistedPools);
        uint64 indexBC = _poolIndex(tokenB, tokenC, whitelistedPools);
        // Calculate expected profits distributed per pool
        uint256 expectedProfitPerPoolAB = _arbitrageProfits[poolIDAB] / 3;
        uint256 expectedProfitPerPoolBC = _arbitrageProfits[poolIDBC] / 3;
        assertEq(_arbitrageProfits[poolIDBC], 30e18);
        // Verify that each pool has received the correct share of profits
        assertEq(profits[indexAB], expectedProfitPerPoolAB, "Pool tokenA/tokenB received incorrect share of profits");
        assertEq(profits[indexBC], expectedProfitPerPoolBC, "Pool tokenB/tokenC received incorrect share of profits");
        //
        // 2. Unwhitelist the tokenB/tokenC pool
        vm.startPrank(address(deployment.dao()));
        deployment.poolsConfig().unwhitelistPool(deployment.pools(), tokenB, tokenC);
        vm.stopPrank();
        //
        // 3. Simulate the profits are sent to the pools. `arbitrageProfits` for poolIDAB is empty, those rewards were sent to the poolIDAB
        vm.prank(address(deployment.upkeep()));
        this.clearProfitsForPools();
        profits = this.profitsForWhitelistedPools();
        assertEq(_arbitrageProfits[poolIDAB], 0);
        assertEq(profits[indexAB], 0);
        //
        // 4. `poolIDBC` still has `arbitrageProfits=30e18` but `profits` does not have the `indexBC` index (index out of bounds).
        assertEq(_arbitrageProfits[poolIDBC], 30e18);
        vm.expectRevert();
        profits[indexBC];
    }

Tools used

Manual review

Recommended Mitigation Steps

Before removing a pool, it is essential to send the arbitrage profits generated by that pool to SaltRewards to ensure that PoolStats::_arbitrageProfits is set to zero when clearProfitsForPools is called. This step will prevent the counting of arbitrage profits that have already been distributed among the POLs and SaltRewards. Implementing this mitigation step will maintain the integrity of arbitrage profit accounting within the system.

Assessed type

Context

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #141

c4-judge commented 9 months ago

Picodes marked the issue as selected for report

c4-sponsor commented 9 months ago

othernet-global (sponsor) acknowledged

c4-sponsor commented 9 months ago

othernet-global (sponsor) disputed

othernet-global commented 9 months ago

If _arbitrageProfits remains for a pool that was unwhitelisted and the pool is later whitelisted again, it is acceptable that the previous _arbitrageProfits value is used to give the pool some share of the pending rewards.

Picodes commented 8 months ago

Flagging this report and its duplicate as a set of duplicate of #752. It's the same root cause and both are of Medium severity.