c4-bot-7 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L200

Vulnerability details

Impact

In some conditions, some pools can lose their arbitrage profits during upkeep to other pools. And small losses will reoccur and accumulate over time.

Proof of Concept

performUpkeep() is permissionless and users are incentivized to call performUpkeep() at any time.

A) In normal conditions, arbitrage profits will be withdrawn to Upkeep.sol in step2. In later steps, arbitrage profits can be combined with emission salt rewards(step6) to be distributed to whitelisted pools that contributed to arbitrage(step7). Then pools' arbitrage profits will be cleared (pools.clearProfitsForPools()).

//src/Upkeep.sol
    function step2(address receiver) public onlySameContract {
         //@audit : when dust amount of arbitrage profits are withdrawn, pools.withdraw() will revert
|>       uint256 withdrawnAmount = exchangeConfig.dao().withdrawArbitrageProfits(
            weth
        );
...
}
    function step6() public onlySameContract {
        //@audit : when timeSinceLastUpkeep==0, emissions.performUpkeep will skip and return
        uint256 timeSinceLastUpkeep = block.timestamp - lastUpkeepTimeEmissions;
        emissions.performUpkeep(timeSinceLastUpkeep);
        lastUpkeepTimeEmissions = block.timestamp;
}
    function step7() public onlySameContract {
        uint256[] memory profitsForPools = pools.profitsForWhitelistedPools();
        bytes32[] memory poolIDs = poolsConfig.whitelistedPools();
        saltRewards.performUpkeep(poolIDs, profitsForPools);
        //@audit : when saltRewards doesn't receive salt to distribute, performUpkeep will skip and return, clearProfitsForPools() clears arbitrage profits storage
|>      pools.clearProfitsForPools();
}

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L200)

B) But in some conditions, (1) there might be no emission salt rewards in step6 if performUpkeep() is called in the same block as the previous upkeep call (timeSinceLastUpkeep==0); (2) And in step2, arbitrage profits might fail to be withdrawn to Upkeep.sol due to dust amount transfer causing reverts, even though there are arbitrage profits.

//src/rewards/Emissions.sol
//step6
    function performUpkeep(uint256 timeSinceLastUpkeep) external
        {
...
|>      if ( timeSinceLastUpkeep == 0 )
            return;
...
}
//src/pools/Pools.sol
//step2
    function withdraw(IERC20 token, uint256 amount) external nonReentrant {
...
         //@audit : when Dao is withdraw dust amount arbitrage profits, withdraw profits will revert, step2 will fail
|>       require(amount > PoolUtils.DUST, "Withdraw amount too small");

The combination of (1) & (2) will cause pools' arbitrage profits records(mapping(bytes32 => uint256) public _arbitrageProfits) to be cleared without receiving rewards. Pools' rewards will be lost.

When Dao's arbitrage profits accumulate later and are distributed, the previously lost small amount might be distributed to different pools that didn't earn the profits from previous upkeeps.

Suppose that many users/bots call performUpkeep() regularly, and when these upkeep transactions settle close to each other, or if arbitrage profits gain is slow due to low trading activities, the above scenario can happen and re-occur.

Tools Used

Manual

Recommended Mitigation Steps

In step7(), (1) consider let saltRewards.performUpkeep(poolIDs, profitsForPools); return 0 when no balance to distribute; (2) Only call pools.clearProfitsForPools() when non-zero is returned from saltRewards.performUpkeep.