Rewards are always lost and locked in GaugeV3.sol due to precision loss in positionPeriodsSecondsInRange() and cachePeriodEarned()

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-10-ramses-exchange/blob/4a40eba36bc47eba8179d4f6203a4b84561a4415/contracts/CL/core/libraries/Position.sol#L261
https://github.com/code-423n4/2024-10-ramses-exchange/blob/4a40eba36bc47eba8179d4f6203a4b84561a4415/contracts/CL/gauge/GaugeV3.sol#L321

Vulnerability details

Proof of Concept

Below provided is a POC with two test cases. The second case proves the periodSecondsInsideX96 of a position is lesser than what it should be due to the precision loss here. The first case proves that since the periodSecondsInsideX96 is lesser and the mulDiv operation in GaugeV3 here is prone to precision loss as well, the ultimate rewards received are lesser, which is asserted in testGaugeIssue as well.

If we assume WETH as the reward token, the loss of rewards would be 16534391534392 wei i.e. 0.000016534391534392 ether, which is 0.04 USD (at current price of USD 2580/ETH). This means that for every claim, we would lose around 0.04 USD on average per claim call, which remains locked in the GaugeV3 contract. After only 10000 claim calls, the loss would be 400 USD. The higher the interactions, the higher the tokens permanently locked in the GaugeV3 contract.

The issue is being deemed as Medium-severity since:

1. This breaks an invariant defined in the README - reward accounting being accurate and not substantially inflated or deflated from reality are crucial.
2. It aligns with C4's Medium-severity classification - There is a leak of value due to the precision loss, which after 10000 claim calls would lead to a reasonable amount of tokens being locked in the GaugeV3.sol contract.

Here are the logs/traces for each test:

• testGaugeIssue

[PASS] testGaugeIssue() (gas: 685344)
Logs:
  Expected rewards: 10000000000000000000
  Realized rewards: 9999983465608465608
  Loss of rewards: 16534391534392

• testPositionPeriodsSecondsInRangeIssue

[PASS] testPositionPeriodsSecondsInRangeIssue() (gas: 482122)
Logs:
  Seconds lost due to casting/rounding: 1416100370

How to use this POC:

• The below foundry test contract uses the existing setup from the test coverage invitational so no additional setup is required other than git cloning the test coverage repo and installing the dependencies.
• Add the POC to the test folder and name it TestPrecisionLoss.sol
• Run the tests using forge test --mt testGaugeIssue -vvvvv and forge test --mt testPositionPeriodsSecondsInRangeIssue -vvvvv
• Both tests display the console.log statements in the traces, where we can see the affected values.
• I've left helpful comments in the POC to help navigate the actions taking place.

//SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;

import {Position} from "../contracts/CL/core/libraries/Position.sol";
import {TickMath} from "../contracts/CL/core/libraries/TickMath.sol";
import {PoolStorage} from "../contracts/CL/core/libraries/PoolStorage.sol";
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";

import {RamsesV3Factory} from "../contracts/CL/core/RamsesV3Factory.sol";
import {RamsesV3Pool} from "../contracts/CL/core/RamsesV3Pool.sol";
import {RamsesV3PoolDeployer} from "../contracts/CL/core/RamsesV3PoolDeployer.sol";
import {GaugeV3} from "../contracts/CL/gauge/GaugeV3.sol";
import {TestERC20} from "../contracts/CL/core/test/TestERC20.sol";

import {Test, console} from "forge-std/Test.sol";

contract TestPrecisionLoss is Test {

    // Contract instances
    RamsesV3Factory factory;
    RamsesV3PoolDeployer poolDeployer;
    RamsesV3Pool pool;
    GaugeV3 gaugeV3;
    TestERC20 tokenA;
    TestERC20 tokenB;

    address public userA = makeAddr("userA");
    address public userB = makeAddr("userB");
    address public userC = makeAddr("userC");

    function setUp() public {
        // Deploy contract instances
        factory = new RamsesV3Factory(address(this));
        poolDeployer = new RamsesV3PoolDeployer(address(factory));
        factory.initialize(address(poolDeployer));

        tokenA = new TestERC20(0);
        tokenB = new TestERC20(0);

        pool = RamsesV3Pool(factory.createPool(address(tokenA), address(tokenB), 1, 10e18));

        gaugeV3 = new GaugeV3(address(this), address(0), address(this), address(pool));
    }

     // @dev Callback function call arriving from mint() to send tokens the pool
     function uniswapV3MintCallback(uint256 amount0Owed, uint256 amount1Owed, bytes calldata data) external {
        //console.log("%e", amount0Owed);
        //console.log(amount1Owed);
        address token0 = pool.token0();
        address token1 = pool.token1();
        TestERC20(token0).mint(address(pool), amount0Owed);
        TestERC20(token1).mint(address(pool), amount1Owed);
    }

    // @dev This contract is the access manager (check restricted modifier in AccessManaged)
     function canCall(address, address, bytes4) external view returns (bool allowed) {
        allowed = true;
    }

    // @dev We directly notify reward through this contract to simplify displaying the real bug
    function collectProtocolFees(address) public {
        return;
    }

    // @dev We just use token0 and token1 as rewards
    function emissionsToken() public returns(address) {
        return address(0);
    }

    function testGaugeIssue() public {
        // Adjust time to enter first period. We start from period = 1 and not period = 0 since periodsInsideX96 is returned as 0 when checkpoint period is 0. 

        vm.warp(0); // Set block.timestamp to 0 directly (foundry starts with 1)
        skip(1 weeks); // Fast forward 1 week + 1 days (i.e. enter into period 1) 

        // Mint positions in first period

        // Assume this position is minted by user A
        uint256 index1 = 0;
        int24 tickLower = TickMath.MIN_TICK;
        int24 tickUpper = TickMath.MAX_TICK;
        pool.mint(userA, index1, tickLower, tickUpper, 10e18, "");

        // Assume rewards are notified to the GaugeV3 contract for current period
        tokenA.mint(address(this), 10e18);
        tokenA.approve(address(gaugeV3), 10e18);
        gaugeV3.notifyRewardAmount(address(tokenA), 10e18);

        skip(7 days);

        address[] memory rewardTokens =  new address[](1);
        rewardTokens[0] = address(tokenA);
        assertEq(0, tokenA.balanceOf(address(this)));
        vm.prank(userA);
        gaugeV3.getPeriodReward(1, rewardTokens, userA, index1, tickLower, tickUpper, userA);
        assertLt(tokenA.balanceOf(userA), 10e18); 
        console.log("Expected rewards:", 10e18);
        console.log("Realized rewards:", tokenA.balanceOf(userA)); 
        console.log("Loss of rewards:", 10e18 - tokenA.balanceOf(userA));
    }

    function testPositionPeriodsSecondsInRangeIssue() public {
        // Adjust time to enter first period. We start from period = 1 and not period = 0 since periodsInsideX96 is returned as 0 when checkpoint period is 0. In reality, this cannot be a bug since it would require block.timestamp to be 0 but good to be aware of.

        vm.warp(0); // Set block.timestamp to 0 directly (foundry starts with 1)
        skip(1 weeks); // Fast forward 1 week (i.e. 1 period) 

        // Mint position in first period

        uint256 index = 0;
        int24 tickLower = TickMath.MIN_TICK;
        int24 tickUpper = TickMath.MAX_TICK;
        pool.mint(address(this), index, tickLower, tickUpper, 10e18, "");

        // Skip time to let the position have some in-range seconds within first period
        skip(1 days);
        pool._advancePeriod();


        // Assert seconds in period using positionPeriodSecondsInRange (should be close to 1 day)
        uint256 periodSecondsInsideX96 = pool.positionPeriodSecondsInRange(1, address(this), index, tickLower, tickUpper);
        assertApproxEqAbs(periodSecondsInsideX96, (1 days) * 2**96, 1e10); 
        assertLt(periodSecondsInsideX96, (1 days) * 2**96);
        console.log("Seconds lost due to casting/rounding:", (1 days) * 2**96 - periodSecondsInsideX96);
    }
}

Recommended Mitigation Steps

Consider introducing a function in GaugeV3, which only allows the accessManager to rescue reward tokens present in the contract, provided the reward token has been removed. Rescuing the tokens should also be handled by the accessManager to ensure decentralized governance during the whole process

Assessed type

Decimal