Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/rewards/Emissions.sol#L47-L50 https://github.com/code-423n4/2024-01-salty/blob/main/src/Upkeep.sol#L187 https://github.com/code-423n4/2024-01-salty/blob/main/src/rewards/RewardsEmitter.sol#L105-L106 https://github.com/code-423n4/2024-01-salty/blob/main/src/Upkeep.sol#L212

Vulnerability details

Impact

Whenever Upkeep from the Upkeep.sol contract is not performed in the Emissions.sol contract at exactly 1 Week Interval, Time and Reward value would be completely Lost.

Proof of Concept

function performUpkeep(uint256 timeSinceLastUpkeep) external
        {
        require( msg.sender == address(exchangeConfig.upkeep()), "Emissions.performUpkeep is only callable from the Upkeep contract" );

        if ( timeSinceLastUpkeep == 0 )
            return;

>>>     // Cap the timeSinceLastUpkeep at one week (if for some reason it has been longer).
        // This will cap the emitted rewards at a default of 0.50% in this transaction.
        if ( timeSinceLastUpkeep >= MAX_TIME_SINCE_LAST_UPKEEP )
>>>         timeSinceLastUpkeep = MAX_TIME_SINCE_LAST_UPKEEP;

        uint256 saltBalance = salt.balanceOf( address( this ) );

        // Target a certain percentage of rewards per week and base what we need to distribute now on how long it has been since the last distribution
        uint256 saltToSend = ( saltBalance * timeSinceLastUpkeep * rewardsConfig.emissionsWeeklyPercentTimes1000() ) / ( 100 * 1000 weeks );
        if ( saltToSend == 0 )
            return;

        // Send the emissions to saltRewards
        salt.safeTransfer(address(saltRewards), saltToSend);
        }

The function above shows how upKeep is handled in the Emissions contract, it can be noted that the only parameter coming into the function is timeSinceLastUpkeep variable, as the name implies it is the value of time since the last time upkeep was called and its later used in the function to calculate the value of salt to be sent. The point of interest from the pointer noted in the code above is that whenever timeSinceLastUpkeep is greater than or equal MAX_TIME_SINCE_LAST_UPKEEP i.e 1 week, it is reduced down to MAX_TIME_SINCE_LAST_UPKEEP or 1 week. Then the important question that should be asked is that what happens to the excess time?, does it have to be lost??

// 6. Send SALT Emissions to the SaltRewards contract.
    function step6() public onlySameContract
        {
        uint256 timeSinceLastUpkeep = block.timestamp - lastUpkeepTimeEmissions;
>>>     emissions.performUpkeep(timeSinceLastUpkeep);

        lastUpkeepTimeEmissions = block.timestamp;
        }

The function above shows how performUpkeep(...) is called from the Upkeep.sol contract, it can be noted that there is no where in this function where the excess time value that would be lost is accounted for, lastUpkeepTimeEmissions simply updates itself to block.timestamp whenever the step6(...) & performUpkeep(...) function is called. This implication of this is that with the cutting short of time value in the Emission contract this also means cutting of reward values that that time is worth.

Working POC

This code could be added to the Emissions.t.sol test contract

function testPerformUpkeepMaxTimeSinceLastUpkeep2() public
    {
 uint256 initialSaltBalance = salt.balanceOf(address(emissions));

// Perform upkeep with a time greater than MAX_TIME_SINCE_LAST_UPKEEP
 vm.prank(address(upkeep));
//emissions.performUpkeep(MAX_TIME_SINCE_LAST_UPKEEP ); ////@using this test will pass
    emissions.performUpkeep(MAX_TIME_SINCE_LAST_UPKEEP + 1);///@but with this test will fail

 // Despite providing a time greater than MAX_TIME_SINCE_LAST_UPKEEP, only MAX_TIME_SINCE_LAST_UPKEEP should be considered
 // Weekly emission rate is .50%, so the expected salt sent is .50% of the initial balance
    uint256 expectedSaltSent = initialSaltBalance * 5 / 1000;
    uint256 finalSaltBalance = salt.balanceOf(address(emissions));
    assertEq(initialSaltBalance - finalSaltBalance, expectedSaltSent);
                 /////////////////////////////////
        /// Consecutive week Upkeep 
        emissions.performUpkeep(MAX_TIME_SINCE_LAST_UPKEEP );

        uint256 expectedSaltSent2 = initialSaltBalance * 10 / 1000;
    uint256 finalSaltBalance2 = salt.balanceOf(address(emissions));
    assertEq(initialSaltBalance - finalSaltBalance2, expectedSaltSent2);

    }

The test would pass and even with the extra time above one week the reward returned is worth of one week, this wouldn't be a problem for the first week but the reward of consequent week will never be the value of one week again, it would be completely lost

Tools Used

Manual Review, Forge Test

Recommended Mitigation Steps

Salty Protocol in the Upkeep.sol contract should update the value of lastUpkeepTimeEmissions by just exactly extra 1 week whenever timeSinceLastUpkeep is in excess, this way when timeSinceLastUpkeep is brought down in the Emission contract it would not affect the next circle of upkeep calculation, so basically what this report is saying is that the excess value does not have to be lost

// 6. Send SALT Emissions to the SaltRewards contract.
    function step6() public onlySameContract
        {
        uint256 timeSinceLastUpkeep = block.timestamp - lastUpkeepTimeEmissions;

        emissions.performUpkeep(timeSinceLastUpkeep);
+++    if ( timeSinceLastUpkeep > 1 week){ 
+++            lastUpkeepTimeEmissions = lastUpkeepTimeEmissions + 1 week;
+++    }
+++    else{
        lastUpkeepTimeEmissions = block.timestamp;
+++     }
        }

This way time value and money value would not be lost when emissions.performUpkeep(...) function is called in the next upkeep round as the new timeSinceLastUpkeep value will not be greater than 1 week under any circumtances and therefore would not be affected by the function below.

function performUpkeep(uint256 timeSinceLastUpkeep) external
        {
        ...
        // Cap the timeSinceLastUpkeep at one week (if for some reason it has been longer).
        // This will cap the emitted rewards at a default of 0.50% in this transaction.
>>>     if ( timeSinceLastUpkeep >= MAX_TIME_SINCE_LAST_UPKEEP ) ///MAX_TIME_SINCE_LAST_UPKEEP  is same as 1 week
>>>         timeSinceLastUpkeep = MAX_TIME_SINCE_LAST_UPKEEP;
  ...

Note: Other Instance of this vulnerability should also be corrected as noted at performUpkeep() & step8() function of the Upkeep contract

Assessed type

Timing

code-423n4 / 2024-01-salty-findings

Time and Salt Value would be Lost whenever Upkeep is not performed at exactly 1 Week Interval #321