User may not receive rewards from `Upkeep:perfomUpkeep` if another transaction frontrun him

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

The Upkeep::performUpkeep function serves to update various aspects of the system. The system rewards the caller with 5% of the arbitrage profits in wETH, as demonstrated in the code snippet below:

File: Upkeep.sol
111:    // 2. Withdraw existing WETH arbitrage profits from the Pools contract and reward the caller of performUpkeep() with default 5% of the withdrawn amount.
112:    function step2(address receiver) public onlySameContract
113:        {
114:        uint256 withdrawnAmount = exchangeConfig.dao().withdrawArbitrageProfits(weth);
115:        if ( withdrawnAmount == 0 )
116:            return;
117: 
118:        // Default 5% of the arbitrage profits for the caller of performUpkeep()
119:        uint256 rewardAmount = withdrawnAmount * daoConfig.upkeepRewardPercent() / 100;
120: 
121:        // Send the reward
122:        weth.safeTransfer(receiver, rewardAmount);
123:        }

Afterward, the remaining wETH is distributed, with 5% going to USDS/DAI Protocol Owned Liquidity, 20% to SALT/USDS Protocol Owned Liquidity, and the remainder sent to SaltRewards. The issue lies in the fact that the caller has no control over whether they receive rewards or not. If another transaction front-runs and captures all the rewards, the caller will receive nothing, having only expended gas. This scenario is quite likely, given that the Upkeep::performUpkeep function can be called by anyone, enabling multiple transactions to be in the mempool, with only the first transaction in the block receiving the rewards.

Proof of Concept

A test was conducted to illustrate how Bob's transaction is executed first in the same block, enabling Bob to claim the rewards. Subsequently, Alice's transaction is executed, but she receives no rewards:

// Filename: src/root_tests/Upkeep.t.sol:TestUpkeep2
// $ forge test --rpc-url https://yourrpc.com --match-test="testPerformUpkeepMultipleTimes" -vv
//
    function testPerformUpkeepMultipleTimes() public {
        // User may not receive rewards if another transaction frontruns it extracting all the available arbitrage profits
        //
        // 1. Mimic depositing arbitrage profits.
        vm.prank(DEPLOYER);
        weth.transfer(address(dao), 100 ether);
        vm.startPrank(address(dao));
        weth.approve(address(pools), 100 ether );
        pools.deposit( weth, 100 ether);
        vm.stopPrank();
        //
        // 2. Bob calls performUpkeep. The bob transaction occurs before the Alice tx. Bob receives 5 wETH as rewards.
        address bob = address(0x2222);
        vm.prank(bob);
        upkeep.performUpkeep();
        assertEq(weth.balanceOf(address(bob)), 5 ether);
        //
        // 3. Alice calls performUpkeep. She does not receive rewards for calling performUpKeep()
        vm.prank(alice);
        upkeep.performUpkeep();
        assertEq(weth.balanceOf(address(alice)), 0);
    }

Tools used

Manual review

Recommended Mitigation Steps

Allow the caller to specify the expected minimum amount of rewards. This would ensure that the caller is guaranteed a reward otherwise the transaction will be reverted and the caller won't expend on gas.

Assessed type

Context

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[c4-sponsor]

othernet-global (sponsor) acknowledged

[c4-sponsor]

othernet-global (sponsor) confirmed

[Picodes]

Downgrading to QA. This is a known problem for "maintenance" functions such as triggering updates or liquidations that distribute rewards to the callers. Even if we add a slippage check the caller can still lose gas. Essentially the caller has a piece of information - calling this function is profitable - that anyone can use so they should never use a public rpc or they will for sure be frontrun.

I consider such keepers to be advanced players and are aware of MEV and will use private mempools or private RPCs to not be frontrun.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)