Holograph contest - 0xA5DF's results

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L415

Vulnerability details

When a beaming job is executed, there's a requirement that the gas left would be at least as the gasLimit set by the user. Given that there's no limit on the gasLimit the user can set, a user can set the gasLimit to amount that's higher than the block gas limit on the dest chain, causing the operator to fail to execute the job.

Impact

Operators would be locked out of the pod, unable to execute any more jobs and not being able to get back the bond they paid.

The attacker would have to pay a value equivalent to the gas fee if that amount was realistic (i.e. gasPrice * gasLimit in dest chain native token), but this can be a relative low amount for Polygon and Avalanche chain (for Polygon that's 20M gas limit and 200 Gwei gas = 4 Matic, for Avalanche the block gas limit seems to be 8M and the price ~30 nAVAX = 0.24 AVAX ). Plus, the operator isn't going to receive that amount.

Proof of Concept

The following test demonstrates this scenario:

diff --git a/test/06_cross-chain_minting_tests_l1_l2.ts b/test/06_cross-chain_minting_tests_l1_l2.ts
index 1f2b959..a1a23b7 100644
--- a/test/06_cross-chain_minting_tests_l1_l2.ts
+++ b/test/06_cross-chain_minting_tests_l1_l2.ts
@@ -276,6 +276,7 @@ describe('Testing cross-chain minting (L1 & L2)', async function () {
             gasLimit: TESTGASLIMIT,
           })
         );
+        estimatedGas = BigNumber.from(50_000_000);
         // process.stdout.write('\n' + 'gas estimation: ' + estimatedGas.toNumber() + '\n');
 
         let payload: BytesLike = await l1.bridge.callStatic.getBridgeOutRequestPayload(
@@ -303,7 +304,8 @@ describe('Testing cross-chain minting (L1 & L2)', async function () {
             '0x' + remove0x((await l1.operator.getMessagingModule()).toLowerCase()).repeat(2),
             payload
           );
-
+        estimatedGas = BigNumber.from(5_000_000);
+        
         process.stdout.write(' '.repeat(10) + 'expected lz gas to be ' + executeJobGas(payload, true).toString());
         await expect(
           adminCall(l2.mockLZEndpoint.connect(l2.lzEndpoint), l2.lzModule, 'lzReceive', [
@@ -313,7 +315,7 @@ describe('Testing cross-chain minting (L1 & L2)', async function () {
             payload,
             {
               gasPrice: GASPRICE,
-              gasLimit: executeJobGas(payload),
+              gasLimit: 5_000_000,
             },
           ])
         )

The test would fail with the following output:

  1) Testing cross-chain minting (L1 & L2)
       Deploy cross-chain contracts via bridge deploy
         hToken
           deploy l1 equivalent on l2:
     VM Exception while processing transaction: revert HOLOGRAPH: not enough gas left

Recommended Mitigation Steps

Limit the gasLimit to the maximum realistic amount that can be used on the dest chain (including the gas used up to the point where it's checked).

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographOperator.sol#L202-L340 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L593-L596 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/module/LayerZeroModule.sol#L277-L294

Vulnerability details

During the beaming process the user compensates the operator for the gas he has to pay by sending some source-chain-native-tokens via hToken. The amount he has to pay is determined according to the gasPrice set by the user, which is supposed to be the maximum gas price to be used on dest chain (therefore predicting the max gas fee the operator would pay and paying him the same value in src chain native tokens). However, in case the user sets a low price (as low as 1 wei) the operator can't skip the job because he's locked out of the pod till he executes the job. The operator would have to choose between loosing money by paying a higher gas fee than he's compensated for or being locked out of the pod - not able to execute additional jobs or get back his bonded amount.

Impact

Operator would be loosing money by having to pay gas fee that's higher than the compensation (gas fee can be a few dozens of USD for heavy txs). This could also be used by attackers to make operators pay for the attackers' expensive gas tasks:

• They can deploy their own contract as the 'source contract'
• Use the bridgeIn event and the data that's being sent to it to instruct the source contract what operations need to be executed
• They can use it for execute operations where the tx.origin doesn't matter (e.g. USDc gasless send)

Proof of Concept

• An operator can't execute any further jobs or leave the pod till the job is executed. From the docs:

When an operator is selected for a job, they are temporarily removed from the pod, until they complete the job. If an operator successfully finalizes a job, they earn a reward and are placed back into their selected pod.

• Operator can't skip a job. Can't prove a negative but that's pretty clear from reading the code.
• There's indeed a third option - that some other operator/user would execute the job instead of the selected operator, but a) the operator would get slashed for that. b) If the compensation is lower than the gas fee then other users have no incentive to execute it as well.

Recommended Mitigation Steps

Allow operator to opt out of executing the job if the gasPrice is higher than the current gas price

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographOperator.sol#L320-L330

Vulnerability details

Jobs might fail due to temporarily conditions, such as an external contract that has been paused, had some bug that was later fixed with an upgrade etc. It can also be a result of an increase of gas needed to execute a job, due to changes to the blockchain state since the gas was calculated (e.g. some cached value has been outdated and needs to recalculated, some condition has been changed and therefore the program flow uses more gas). Therefore, it's important to allow users to execute failed jobs once the reason that caused the job to fail no longer exists.

Impact

Tokens of jobs that failed due to temporarily conditions would be lost forever.

Proof of Concept

As can be seen in the code, in case of a failure all that is done is saving the job to the _failedJobs mapping. Nothing is done with _failedJobs later.

   try
      HolographOperatorInterface(address(this)).nonRevertingBridgeCall{value: msg.value}(
        msg.sender,
        bridgeInRequestPayload
      )
    {
      /// @dev do nothing
    } catch {
      _failedJobs[hash] = true;
      emit FailedOperatorJob(hash);
    }

Recommended Mitigation Steps

Add functionality that will allow any user to run a failed job till it succeeds.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographOperator.sol#L316

Vulnerability details

There's a check at line 316 that verifies that there's enough gas left to execute the HolographBridge.bridgeInRequest() with the gasLimit set by the user, however the actual amount of gas left during the call is less than that (mainly due to the 1/64 rule, see below). An attacker can use that gap to fail the job while still having the executeJob() function complete.

Impact

The owner of the bridged token would loose access to the token since the job failed.

Proof of Concept

Besides using a few units of gas between the check and the actual call, there's also a rule that only 63/64 of the remaining gas would be dedicated to an (external) function call. Since there are 2 external function calls done (nonRevertingBridgeCall() and the actual call to the bridge) ~2/64 of the gas isn't sent to the bridge call and can be used after the bridge call runs out of gas.

The following PoC shows that if the amount of gas left before the call is at least 1 million then the execution can continue after the bridge call fails:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import "forge-std/Test.sol";

contract ContractTest is Test {
    event FailedOperatorJob(bytes32 jobHash);
    uint256 private _inboundMessageCounter;
    mapping(bytes32 => bool) private _failedJobs;
    constructor(){
        _inboundMessageCounter = 5;
    }
    function testGas64() public {
        this.entryPoint{gas:1000000}();
    }

    Bridge bridge = new Bridge();
    event GasLeftAfterFail(uint left);

    function entryPoint() public {

        console2.log("Gas left before call: ", gasleft());

        bytes32 hash = 0x987744358512a04274ccfb3d9649da3c116cd6b19c535e633ef8529a80cb06a0;

        try this.intermediate(){
        }catch{
            // check out how much gas is left after the call to the bridge failed
            console2.log("Gas left after failure: ", gasleft());
            // simulate operations done after failure
            _failedJobs[hash] = true;
            emit FailedOperatorJob(hash);
        }
        ++_inboundMessageCounter;
        console2.log("Gas left at end: ", gasleft());

    }

    function intermediate() public{
        bridge.bridgeCall();
    }
}


contract Bridge{
    event Done(uint gasLeft);

    uint256[] myArr;

    function bridgeCall() public {
        for(uint i =1; i <= 100; i++){
            myArr.push(i);
        }
        // this line would never be reached, we'll be out of gas beforehand
        emit Done(gasleft());
    }
}

Output of PoC:

  Gas left before call:  999772
  Gas left after failure:  30672
  Gas left at end:  1628

Side note: due to some bug in forge _inboundMessageCounter would be considered warm even though it's not necessarily the case. However in a real world scenario we can warm it up if the selected operator is a contract and we'er using another operator contract to execute a job in the same tx beforehand.

Reference for the 1/64 rule - EIP-150. Also check out evm.codes.

Recommended Mitigation Steps

Modify the required amount of gas left to gasLimit + any amount of gas spent before reaching the call(), then multiply it by 32/30 to mitigate the 1/64 rule (+ some margin of safety maybe).

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographOperator.sol#L255

Vulnerability details

Impact

If the following conditions have been met:

• The selected operator doesn't complete the job, either intentionally (they're sacrificing their bonded amount to harm the token owner) or innocently (hardware failure that caused a loss of access to the wallet)
• Gas price has spiked, and isn't going down than the gasPrice set by the user in the bridge out request

Then the bridging request wouldn't complete and the token owner would loos access to the token till the gas price goes back down again.

Proof of Concept

The fact that no one but the selected operator can execute the job in case of a gas spike has been proven by the test 'Should fail if there has been a gas spike' provided by the sponsor.

An example of a price spike can be in the recent month in the Ethereum Mainnet where the min gas price was 3 at Oct 8, but jumped to 14 the day after and didn't go down since then (the min on Oct 9 was lower than the avg of Oct8, but users might witness a momentarily low gas price and try to hope on it). See the gas price chat on Etherscan for more details.

Recommended Mitigation Steps

In case of a gas price spike, instead of refusing to let other operators to execute the job, let them execute the job without slashing the selected operator. This way, after a while also the owner can execute the job and pay the gas price.