getL2Output reverts if not found (#2979)

* feat(bedrock): getL2Output reverts if not found

feat(bedrock): Handle error on empty output in Portal

feat(ctb): use isOutputFinalized in finalizeWithdrawalTx

* ctb: Remove extraneous abi encoding

Revert "feat(ctb): use isOutputFinalized in finalizeWithdrawalTx"

This reverts commit 4d4e89d4f624b8cbd95c1ad2800027bfe561c213.

ctb: getL2Output returns the next output for a block

ctb: Fix error message style

.changeset/hot-queens-accept.md

+---
+'@eth-optimism/contracts-bedrock': patch
+---
+
+The output oracle's getL2Output function now reverts when no output is returned

packages/contracts-bedrock/.gas-snapshot

@@ -39,17 +39,17 @@ L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 24494)
 L1CrossDomainMessenger_Test:testCannot_L1MessengerUnpause() (gas: 24530)
 L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 24704)
 L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 47993)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 201871)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageRevertsOnReentrancy() (gas: 195009)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 77751)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 67873)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 201937)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageRevertsOnReentrancy() (gas: 195075)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 77817)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 67939)
 L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 60448)
 L1CrossDomainMessenger_Test:test_L1MessengerReplayMessageWithValue() (gas: 38160)
 L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 298101)
 L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 1489673)
 L1CrossDomainMessenger_Test:test_L1MessengerUnpause() (gas: 40852)
 L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 24313)
-L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 86298)
+L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 86364)
 L1StandardBridge_Test:test_depositERC20() (gas: 578548)
 L1StandardBridge_Test:test_depositERC20To() (gas: 580752)
 L1StandardBridge_Test:test_depositETH() (gas: 372464)
@@ -76,9 +76,9 @@ L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 133074)
 L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10612)
 L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54893)
 L2OutputOracleTest:testCannot_ProposeWithUnmatchedBlockhash() (gas: 26829)
-L2OutputOracleTest:testCannot_deleteL2Output_ifNotOwner() (gas: 24842)
-L2OutputOracleTest:testCannot_deleteL2Output_withWrongRoot() (gas: 91177)
-L2OutputOracleTest:testCannot_deleteL2Output_withWrongTime() (gas: 87171)
+L2OutputOracleTest:testCannot_deleteL2Output_ifNotOwner() (gas: 25097)
+L2OutputOracleTest:testCannot_deleteL2Output_withWrongRoot() (gas: 91432)
+L2OutputOracleTest:testCannot_deleteL2Output_withWrongTime() (gas: 87426)
 L2OutputOracleTest:testCannot_proposeEmptyOutput() (gas: 24128)
 L2OutputOracleTest:testCannot_proposeFutureTimetamp() (gas: 26097)
 L2OutputOracleTest:testCannot_proposeL2OutputIfNotProposer() (gas: 23564)
@@ -86,9 +86,9 @@ L2OutputOracleTest:testCannot_proposeOnWrongFork() (gas: 26424)
 L2OutputOracleTest:testCannot_proposeUnexpectedBlockNumber() (gas: 25983)
 L2OutputOracleTest:test_changeProposer() (gas: 56052)
 L2OutputOracleTest:test_computeL2Timestamp() (gas: 30288)
-L2OutputOracleTest:test_constructor() (gas: 48791)
-L2OutputOracleTest:test_deleteOutput() (gas: 76611)
-L2OutputOracleTest:test_getL2Output() (gas: 82934)
+L2OutputOracleTest:test_constructor() (gas: 49046)
+L2OutputOracleTest:test_deleteOutput() (gas: 77223)
+L2OutputOracleTest:test_getL2Output() (gas: 88514)
 L2OutputOracleTest:test_latestBlockNumber() (gas: 76284)
 L2OutputOracleTest:test_nextBlockNumber() (gas: 15232)
 L2OutputOracleTest:test_proposeWithBlockhashAndHeight() (gas: 75046)
@@ -96,7 +96,7 @@ L2OutputOracleTest:test_proposingAnotherOutput() (gas: 76881)
 L2OutputOracleTest:test_updateOwner() (gas: 34580)
 L2OutputOracleUpgradeable_Test:test_cannotInitImpl() (gas: 19428)
 L2OutputOracleUpgradeable_Test:test_cannotInitProxy() (gas: 24427)
-L2OutputOracleUpgradeable_Test:test_initValuesOnProxy() (gas: 38831)
+L2OutputOracleUpgradeable_Test:test_initValuesOnProxy() (gas: 39086)
 L2OutputOracleUpgradeable_Test:test_upgrading() (gas: 230843)
 L2StandardBridge_Test:test_ERC20BridgeFailed_whenLocalTokenIsBridge() (gas: 132769)
 L2StandardBridge_Test:test_cannotWithdrawEthWithoutSendingIt() (gas: 21622)
@@ -133,14 +133,14 @@ OptimismMintableTokenFactory_Test:test_bridge() (gas: 7663)
 OptimismMintableTokenFactory_Test:test_createStandardL2Token() (gas: 1113127)
 OptimismMintableTokenFactory_Test:test_createStandardL2TokenSameTwice() (gas: 2209165)
 OptimismMintableTokenFactory_Test:test_createStandardL2TokenShouldRevertIfRemoteIsZero() (gas: 9398)
-OptimismPortalUpgradeable_Test:test_cannotInitImpl() (gas: 10708)
-OptimismPortalUpgradeable_Test:test_cannotInitProxy() (gas: 15684)
+OptimismPortalUpgradeable_Test:test_cannotInitImpl() (gas: 10686)
+OptimismPortalUpgradeable_Test:test_cannotInitProxy() (gas: 15662)
 OptimismPortalUpgradeable_Test:test_initValuesOnProxy() (gas: 15967)
 OptimismPortalUpgradeable_Test:test_upgrading() (gas: 230843)
-OptimismPortal_Test:test_OptimismPortalConstructor() (gas: 17275)
+OptimismPortal_Test:test_OptimismPortalConstructor() (gas: 17341)
 OptimismPortal_Test:test_OptimismPortalContractCreationReverts() (gas: 14215)
 OptimismPortal_Test:test_OptimismPortalReceiveEth() (gas: 127503)
-OptimismPortal_Test:test_cannotVerifyRecentWithdrawal() (gas: 31946)
+OptimismPortal_Test:test_cannotFinalizeRecentWithdrawal() (gas: 24754)
 OptimismPortal_Test:test_depositTransaction_NoValueContract() (gas: 76654)
 OptimismPortal_Test:test_depositTransaction_NoValueEOA() (gas: 77108)
 OptimismPortal_Test:test_depositTransaction_createWithZeroValueForContract() (gas: 76659)
@@ -148,10 +148,10 @@ OptimismPortal_Test:test_depositTransaction_createWithZeroValueForEOA() (gas: 76
 OptimismPortal_Test:test_depositTransaction_withEthValueAndContractContractCreation() (gas: 83680)
 OptimismPortal_Test:test_depositTransaction_withEthValueAndEOAContractCreation() (gas: 75845)
 OptimismPortal_Test:test_depositTransaction_withEthValueFromContract() (gas: 83384)
-OptimismPortal_Test:test_depositTransaction_withEthValueFromEOA() (gas: 84154)
-OptimismPortal_Test:test_invalidWithdrawalProof() (gas: 45218)
-OptimismPortal_Test:test_isOutputFinalized() (gas: 132035)
-OptimismPortal_Test:test_simple_isOutputFinalized() (gas: 24019)
+OptimismPortal_Test:test_depositTransaction_withEthValueFromEOA() (gas: 84132)
+OptimismPortal_Test:test_invalidWithdrawalProof() (gas: 37266)
+OptimismPortal_Test:test_isBlockFinalized() (gas: 113725)
+OptimismPortal_Test:test_simple_isBlockFinalized() (gas: 26652)
 Proxy_Test:test_clashingFunctionSignatures() (gas: 101427)
 Proxy_Test:test_implementationKey() (gas: 20942)
 Proxy_Test:test_implementationProxyCallIfNotAdmin() (gas: 30021)

packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol

@@ -260,13 +260,35 @@ contract L2OutputOracle is OwnableUpgradeable, Semver {
     }
 
     /**
-     * @notice Returns the L2 output proposal given a target L2 block number.
-     *         Returns a null output proposal if none is found.
+     * @notice Returns the L2 output proposal associated with a target L2 block number. If the
+     *         L2 block number provided is between checkpoints, this function will rerutn the next
+     *         proposal for the next checkpoint.
+     *         Reverts if the output proposal is either not found, or predates
+     *         the STARTING_BLOCK_NUMBER.
      *
      * @param _l2BlockNumber The L2 block number of the target block.
      */
     function getL2Output(uint256 _l2BlockNumber) external view returns (OutputProposal memory) {
-        return l2Outputs[_l2BlockNumber];
+        require(
+            _l2BlockNumber >= STARTING_BLOCK_NUMBER,
+            "L2OutputOracle: block number cannot be less than the starting block number."
+        );
+
+        // Find the distance between _l2BlockNumber, and the checkpoint block before it.
+        uint256 offset = (_l2BlockNumber - STARTING_BLOCK_NUMBER) % SUBMISSION_INTERVAL;
+
+        // If the offset is zero, then the _l2BlockNumber should be checkpointed.
+        // Otherwise, we'll look up the next block that will be checkpointed.
+        uint256 lookupBlockNumber = offset == 0
+            ? _l2BlockNumber
+            : _l2BlockNumber + (SUBMISSION_INTERVAL - offset);
+
+        OutputProposal memory output = l2Outputs[lookupBlockNumber];
+        require(
+            output.outputRoot != bytes32(0),
+            "L2OutputOracle: No output found for that block number."
+        );
+        return output;
     }
 
     /**

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol

@@ -169,33 +169,28 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
     }
 
     /**
-     * @notice Determine if an L2 Output is finalized.
+     * @notice Determine if a given block number is finalized. Reverts if the call to
+     *         L2_ORACLE.getL2Output reverts. Returns a boolean otherwise.
      *
      * @param _l2BlockNumber The number of the L2 block.
      */
-
-    function isOutputFinalized(uint256 _l2BlockNumber) external view returns (bool) {
+    function isBlockFinalized(uint256 _l2BlockNumber) external view returns (bool) {
         L2OutputOracle.OutputProposal memory proposal = L2_ORACLE.getL2Output(_l2BlockNumber);
 
-        if (proposal.outputRoot == bytes32(uint256(0))) {
-            uint256 interval = L2_ORACLE.SUBMISSION_INTERVAL();
-            uint256 startingBlockNumber = L2_ORACLE.STARTING_BLOCK_NUMBER();
-
-            // Prevent underflow
-            if (startingBlockNumber > _l2BlockNumber) {
-                return false;
-            }
-
-            // Find the distance between the _l2BlockNumber, and the checkpoint block before it.
-            uint256 offset = (_l2BlockNumber - startingBlockNumber) % interval;
-            // Look up the checkpoint block after it.
-            proposal = L2_ORACLE.getL2Output(_l2BlockNumber + (interval - offset));
-            // False if that block is not yet proposed.
-            if (proposal.outputRoot == bytes32(uint256(0))) {
-                return false;
-            }
-        }
-        return block.timestamp > proposal.timestamp + FINALIZATION_PERIOD_SECONDS;
+        return _isOutputFinalized(proposal);
+    }
+
+    /**
+     * @notice Determine if an L2 Output is finalized.
+     *
+     * @param _proposal The output proposal to check.
+     */
+    function _isOutputFinalized(L2OutputOracle.OutputProposal memory _proposal)
+        internal
+        view
+        returns (bool)
+    {
+        return block.timestamp > _proposal.timestamp + FINALIZATION_PERIOD_SECONDS;
     }
 
     /**
@@ -235,17 +230,15 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
             "OptimismPortal: you cannot send messages to the portal contract"
         );
 
-        // Get the output root.
+        // Get the output root. This will fail if there is no
+        // output root for the given block number.
         L2OutputOracle.OutputProposal memory proposal = L2_ORACLE.getL2Output(_l2BlockNumber);
 
         // Ensure that enough time has passed since the proposal was submitted before allowing a
         // withdrawal. Under the assumption that the fault proof mechanism is operating correctly,
         // we can infer that any withdrawal that has passed the finalization period must be valid
         // and can therefore be operated on.
-        require(
-            block.timestamp > proposal.timestamp + FINALIZATION_PERIOD_SECONDS,
-            "OptimismPortal: proposal is not yet finalized"
-        );
+        require(_isOutputFinalized(proposal), "OptimismPortal: proposal is not yet finalized");
 
         // Verify that the output root can be generated with the elements in the proof.
         require(

packages/contracts-bedrock/contracts/test/L2OutputOracle.t.sol

@@ -54,9 +54,18 @@ contract L2OutputOracleTest is L2OutputOracle_Initializer {
         assertEq(proposal.outputRoot, proposedOutput1);
         assertEq(proposal.timestamp, block.timestamp);
 
-        L2OutputOracle.OutputProposal memory proposal2 = oracle.getL2Output(0);
-        assertEq(proposal2.outputRoot, bytes32(0));
-        assertEq(proposal2.timestamp, 0);
+        // Handles a block number that is between checkpoints:
+        proposal = oracle.getL2Output(nextBlockNumber - 1);
+        assertEq(proposal.outputRoot, proposedOutput1);
+        assertEq(proposal.timestamp, block.timestamp);
+
+        // The block number is too low:
+        vm.expectRevert("L2OutputOracle: block number cannot be less than the starting block number.");
+        proposal = oracle.getL2Output(0);
+
+        // The block number is larger than the latest proposed output:
+        vm.expectRevert("L2OutputOracle: No output found for that block number.");
+        proposal = oracle.getL2Output(nextBlockNumber + 1);
     }
 
     // Test: nextBlockNumber() should return the correct value

packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol

@@ -220,7 +220,7 @@ contract OptimismPortal_Test is Portal_Initializer {
     // TODO: test this deeply
     // function test_verifyWithdrawal() external {}
 
-    function test_cannotVerifyRecentWithdrawal() external {
+    function test_cannotFinalizeRecentWithdrawal() external {
         Hashing.OutputRootProof memory outputRootProof = Hashing
             .OutputRootProof({
                 version: bytes32(0),
@@ -228,12 +228,24 @@ contract OptimismPortal_Test is Portal_Initializer {
                 withdrawerStorageRoot: bytes32(0),
                 latestBlockhash: bytes32(0)
             });
+        // Setup the Oracle to return an output with a recent timestamp
+        uint256 recentTimestamp = block.timestamp - 1000;
+        vm.mockCall(
+            address(op.L2_ORACLE()),
+            abi.encodeWithSelector(L2OutputOracle.getL2Output.selector),
+            abi.encode(L2OutputOracle.OutputProposal(bytes32(uint256(1)), recentTimestamp))
+        );
 
         vm.expectRevert("OptimismPortal: proposal is not yet finalized");
         op.finalizeWithdrawalTransaction(0, alice, alice, 0, 0, hex"", 0, outputRootProof, hex"");
     }
 
     function test_invalidWithdrawalProof() external {
+        vm.mockCall(
+            address(op.L2_ORACLE()),
+            abi.encodeWithSelector(L2OutputOracle.getL2Output.selector),
+            abi.encode(L2OutputOracle.OutputProposal(bytes32(uint256(1)), block.timestamp))
+        );
         Hashing.OutputRootProof memory outputRootProof = Hashing
             .OutputRootProof({
                 version: bytes32(0),
@@ -243,17 +255,15 @@ contract OptimismPortal_Test is Portal_Initializer {
             });
 
         vm.warp(
-            oracle.getL2Output(
-                oracle.latestBlockNumber()
-            ).timestamp
-            + op.FINALIZATION_PERIOD_SECONDS()
+            oracle.getL2Output(oracle.latestBlockNumber()).timestamp +
+                op.FINALIZATION_PERIOD_SECONDS() + 1
         );
 
         vm.expectRevert("OptimismPortal: invalid output root proof");
         op.finalizeWithdrawalTransaction(0, alice, alice, 0, 0, hex"", 0, outputRootProof, hex"");
     }
 
-    function test_simple_isOutputFinalized() external {
+    function test_simple_isBlockFinalized() external {
         vm.mockCall(
             address(op.L2_ORACLE()),
             abi.encodeWithSelector(
@@ -262,20 +272,20 @@ contract OptimismPortal_Test is Portal_Initializer {
             abi.encode(
                 L2OutputOracle.OutputProposal(
                     bytes32(uint256(1)),
-                    0
+                    startingBlockNumber
                 )
             )
         );
 
         // warp to the finalization period
-        vm.warp(op.FINALIZATION_PERIOD_SECONDS());
-        assertEq(op.isOutputFinalized(0), false);
+        vm.warp(startingBlockNumber + op.FINALIZATION_PERIOD_SECONDS());
+        assertEq(op.isBlockFinalized(startingBlockNumber), false);
         // warp past the finalization period
-        vm.warp(op.FINALIZATION_PERIOD_SECONDS() + 1);
-        assertEq(op.isOutputFinalized(0), true);
+        vm.warp(startingBlockNumber + op.FINALIZATION_PERIOD_SECONDS() + 1);
+        assertEq(op.isBlockFinalized(startingBlockNumber), true);
     }
 
-    function test_isOutputFinalized() external {
+    function test_isBlockFinalized() external {
         uint256 checkpoint = oracle.nextBlockNumber();
         vm.roll(checkpoint);
         vm.warp(oracle.computeL2Timestamp(checkpoint) + 1);
@@ -286,21 +296,23 @@ contract OptimismPortal_Test is Portal_Initializer {
         uint256 finalizationHorizon = block.timestamp + op.FINALIZATION_PERIOD_SECONDS();
         vm.warp(finalizationHorizon);
         // The checkpointed block should not be finalized until 1 second from now.
-        assertEq(op.isOutputFinalized(checkpoint), false);
+        assertEq(op.isBlockFinalized(checkpoint), false);
         // Nor should a block after it
-        assertEq(op.isOutputFinalized(checkpoint + 1), false);
+        vm.expectRevert("L2OutputOracle: No output found for that block number.");
+        assertEq(op.isBlockFinalized(checkpoint + 1), false);
         // Nor a block before it, even though the finalization period has passed, there is
         // not yet a checkpoint block on top of it for which that is true.
-        assertEq(op.isOutputFinalized(checkpoint - 1), false);
+        assertEq(op.isBlockFinalized(checkpoint - 1), false);
 
         // warp past the finalization period
         vm.warp(finalizationHorizon + 1);
         // It should now be finalized.
-        assertEq(op.isOutputFinalized(checkpoint), true);
+        assertEq(op.isBlockFinalized(checkpoint), true);
         // So should the block before it.
-        assertEq(op.isOutputFinalized(checkpoint - 1), true);
+        assertEq(op.isBlockFinalized(checkpoint - 1), true);
         // But not the block after it.
-        assertEq(op.isOutputFinalized(checkpoint + 1), false);
+        vm.expectRevert("L2OutputOracle: No output found for that block number.");
+        assertEq(op.isBlockFinalized(checkpoint + 1), false);
     }
 }