fix: abi encoded tx

.changeset/witty-chefs-learn.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts': patch
+---
+
+Do not RLP decode the transaction in the OVM_ECDSAContractAccount

packages/contracts/contracts/optimistic-ethereum/OVM/accounts/OVM_ECDSAContractAccount.sol

@@ -19,7 +19,7 @@ import { SafeMath } from "@openzeppelin/contracts/math/SafeMath.sol";
 /**
  * @title OVM_ECDSAContractAccount
  * @dev The ECDSA Contract Account can be used as the implementation for a ProxyEOA deployed by the
- * ovmCREATEEOA operation. It enables backwards compatibility with Ethereum's Layer 1, by 
+ * ovmCREATEEOA operation. It enables backwards compatibility with Ethereum's Layer 1, by
  * providing EIP155 formatted transaction encodings.
  *
  * Compiler used: optimistic-solc
@@ -49,12 +49,12 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
 
     /**
      * Executes a signed transaction.
-     * @param _encodedTransaction Signed EIP155 transaction.
+     * @param _transaction Signed EIP155 transaction.
      * @return Whether or not the call returned (rather than reverted).
      * @return Data returned by the call.
      */
     function execute(
-        bytes memory _encodedTransaction
+        Lib_EIP155Tx.EIP155Tx memory _transaction
     )
         override
         public
@@ -63,23 +63,22 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
             bytes memory
         )
     {
-        // Attempt to decode the transaction.
-        Lib_EIP155Tx.EIP155Tx memory transaction = Lib_EIP155Tx.decode(
-            _encodedTransaction,
-            Lib_ExecutionManagerWrapper.ovmCHAINID()
-        );
-
         // Address of this contract within the ovm (ovmADDRESS) should be the same as the
         // recovered address of the user who signed this message. This is how we manage to shim
         // account abstraction even though the user isn't a contract.
         require(
-            transaction.sender() == Lib_ExecutionManagerWrapper.ovmADDRESS(),
+            _transaction.sender() == Lib_ExecutionManagerWrapper.ovmADDRESS(),
             "Signature provided for EOA transaction execution is invalid."
         );
 
+        require(
+            _transaction.chainId == Lib_ExecutionManagerWrapper.ovmCHAINID(),
+            "Transaction signed with wrong chain ID"
+        );
+
         // Need to make sure that the transaction nonce is right.
         require(
-            transaction.nonce == Lib_ExecutionManagerWrapper.ovmGETNONCE(),
+            _transaction.nonce == Lib_ExecutionManagerWrapper.ovmGETNONCE(),
             "Transaction nonce does not match the expected nonce."
         );
 
@@ -94,20 +93,20 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
         require(
             OVM_ETH(Lib_PredeployAddresses.OVM_ETH).transfer(
                 Lib_PredeployAddresses.SEQUENCER_FEE_WALLET,
-                SafeMath.mul(transaction.gasLimit, transaction.gasPrice)
+                SafeMath.mul(_transaction.gasLimit, _transaction.gasPrice)
             ),
             "Fee was not transferred to relayer."
         );
 
-        if (transaction.isCreate) {
+        if (_transaction.isCreate) {
             // TEMPORARY: Disable value transfer for contract creations.
             require(
-                transaction.value == 0,
+                _transaction.value == 0,
                 "Value transfer in contract creation not supported."
             );
 
             (address created, bytes memory revertdata) = Lib_ExecutionManagerWrapper.ovmCREATE(
-                transaction.data
+                _transaction.data
             );
 
             // Return true if the contract creation succeeded, false w/ revertdata otherwise.
@@ -123,17 +122,17 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
             Lib_ExecutionManagerWrapper.ovmINCREMENTNONCE();
 
             // Value transfer currently only supported for CALL but not for CREATE.
-            if (transaction.value > 0) {
+            if (_transaction.value > 0) {
                 // TEMPORARY: Block value transfer if the transaction has input data.
                 require(
-                    transaction.data.length == 0,
+                    _transaction.data.length == 0,
                     "Value is nonzero but input data was provided."
                 );
 
                 require(
                     OVM_ETH(Lib_PredeployAddresses.OVM_ETH).transfer(
-                        transaction.to,
-                        transaction.value
+                        _transaction.to,
+                        _transaction.value
                     ),
                     "Value could not be transferred to recipient."
                 );
@@ -144,11 +143,11 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
                 // so that they don't have to pay any fees to the sequencer. Function will remain disabled
                 // until a robust solution is in place.
                 require(
-                    transaction.to != Lib_ExecutionManagerWrapper.ovmADDRESS(),
+                    _transaction.to != Lib_ExecutionManagerWrapper.ovmADDRESS(),
                     "Calls to self are disabled until upgradability is re-enabled."
                 );
 
-                return transaction.to.call(transaction.data);
+                return _transaction.to.call(_transaction.data);
             }
         }
     }

packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol

@@ -1,9 +1,11 @@
 // SPDX-License-Identifier: MIT
 pragma solidity >0.5.0 <0.8.0;
+pragma experimental ABIEncoderV2;
 
 /* Library Imports */
 import { Lib_EIP155Tx } from "../../libraries/codec/Lib_EIP155Tx.sol";
 import { Lib_ExecutionManagerWrapper } from "../../libraries/wrappers/Lib_ExecutionManagerWrapper.sol";
+import { iOVM_ECDSAContractAccount } from "../../iOVM/accounts/iOVM_ECDSAContractAccount.sol";
 
 /**
  * @title OVM_SequencerEntrypoint
@@ -63,10 +65,7 @@ contract OVM_SequencerEntrypoint {
 
         // Forward the transaction over to the EOA.
         (bool success, bytes memory returndata) = target.call(
-            abi.encodeWithSignature(
-                "execute(bytes)",
-                encodedTx
-            )
+            abi.encodeWithSelector(iOVM_ECDSAContractAccount.execute.selector, transaction)
         );
 
         if (success) {

packages/contracts/contracts/optimistic-ethereum/iOVM/accounts/iOVM_ECDSAContractAccount.sol

@@ -3,7 +3,7 @@ pragma solidity >0.5.0 <0.8.0;
 pragma experimental ABIEncoderV2;
 
 /* Library Imports */
-import { Lib_OVMCodec } from "../../libraries/codec/Lib_OVMCodec.sol";
+import { Lib_EIP155Tx } from "../../libraries/codec/Lib_EIP155Tx.sol";
 
 /**
  * @title iOVM_ECDSAContractAccount
@@ -15,7 +15,7 @@ interface iOVM_ECDSAContractAccount {
      ********************/
 
     function execute(
-        bytes memory _encodedTransaction
+        Lib_EIP155Tx.EIP155Tx memory _transaction
     )
         external
         returns (

packages/contracts/contracts/optimistic-ethereum/libraries/codec/Lib_EIP155Tx.sol

@@ -48,12 +48,12 @@ library Lib_EIP155Tx {
         // didn't use a uint8, then recovery_parameter would always be a negative number for chain
         // IDs greater than 110 (`255 - 110 * 2 - 35 = 0`). So we need to wrap around to support
         // anything larger.
-        uint8 recoveryParam; 
+        uint8 recoveryParam;
 
         // Whether or not the transaction is a creation. Necessary because we can't make an address
         // "nil". Using the zero address creates a potential conflict if the user did actually
         // intend to send a transaction to the zero address.
-        bool isCreate;       
+        bool isCreate;
     }
 
     // Lets us use nicer syntax.
@@ -152,7 +152,7 @@ library Lib_EIP155Tx {
             raw[8] = Lib_RLPWriter.writeBytes32(_transaction.s);
         } else {
             // Chain ID *is* included in the unsigned transaction.
-            raw[6] = Lib_RLPWriter.writeUint(_transaction.chainId); 
+            raw[6] = Lib_RLPWriter.writeUint(_transaction.chainId);
             raw[7] = Lib_RLPWriter.writeBytes('');
             raw[8] = Lib_RLPWriter.writeBytes('');
         }

packages/contracts/package.json

@@ -60,6 +60,7 @@
     "@codechecks/client": "0.1.10-beta",
     "@eth-optimism/hardhat-ovm": "^0.2.2",
     "@eth-optimism/smock": "^1.1.3",
+    "@ethersproject/transactions": "^5.0.31",
     "@nomiclabs/hardhat-ethers": "^2.0.1",
     "@nomiclabs/hardhat-waffle": "^2.0.1",
     "@openzeppelin/contracts": "^3.3.0",

packages/contracts/test/contracts/OVM/accounts/OVM_ECDSAContractAccount.spec.ts

@@ -7,7 +7,7 @@ import { MockContract, smockit } from '@eth-optimism/smock'
 import { toPlainObject } from 'lodash'
 
 /* Internal Imports */
-import { DEFAULT_EIP155_TX } from '../../../helpers'
+import { LibEIP155TxStruct, DEFAULT_EIP155_TX } from '../../../helpers'
 import { predeploys } from '../../../../src'
 
 describe('OVM_ECDSAContractAccount', () => {
@@ -54,14 +54,18 @@ describe('OVM_ECDSAContractAccount', () => {
       const transaction = DEFAULT_EIP155_TX
       const encodedTransaction = await wallet.signTransaction(transaction)
 
-      await OVM_ECDSAContractAccount.execute(encodedTransaction)
+      await OVM_ECDSAContractAccount.execute(
+        LibEIP155TxStruct(encodedTransaction)
+      )
     })
 
     it(`should ovmCREATE if EIP155Transaction.to is zero address`, async () => {
       const transaction = { ...DEFAULT_EIP155_TX, to: '' }
       const encodedTransaction = await wallet.signTransaction(transaction)
 
-      await OVM_ECDSAContractAccount.execute(encodedTransaction)
+      await OVM_ECDSAContractAccount.execute(
+        LibEIP155TxStruct(encodedTransaction)
+      )
 
       const ovmCREATE: any =
         Mock__OVM_ExecutionManager.smocked.ovmCREATE.calls[0]
@@ -76,7 +80,7 @@ describe('OVM_ECDSAContractAccount', () => {
       )
 
       await expect(
-        OVM_ECDSAContractAccount.execute(encodedTransaction)
+        OVM_ECDSAContractAccount.execute(LibEIP155TxStruct(encodedTransaction))
       ).to.be.revertedWith(
         'Signature provided for EOA transaction execution is invalid.'
       )
@@ -87,7 +91,7 @@ describe('OVM_ECDSAContractAccount', () => {
       const encodedTransaction = await wallet.signTransaction(transaction)
 
       await expect(
-        OVM_ECDSAContractAccount.execute(encodedTransaction)
+        OVM_ECDSAContractAccount.execute(LibEIP155TxStruct(encodedTransaction))
       ).to.be.revertedWith(
         'Transaction nonce does not match the expected nonce.'
       )
@@ -98,19 +102,18 @@ describe('OVM_ECDSAContractAccount', () => {
       const encodedTransaction = await wallet.signTransaction(transaction)
 
       await expect(
-        OVM_ECDSAContractAccount.execute(encodedTransaction)
-      ).to.be.revertedWith(
-        'Lib_EIP155Tx: Transaction signed with wrong chain ID'
-      )
+        OVM_ECDSAContractAccount.execute(LibEIP155TxStruct(encodedTransaction))
+      ).to.be.revertedWith('Transaction signed with wrong chain ID')
     })
 
     // TEMPORARY: Skip gas checks for minnet.
     it.skip(`should revert on insufficient gas`, async () => {
       const transaction = { ...DEFAULT_EIP155_TX, gasLimit: 200000000 }
       const encodedTransaction = await wallet.signTransaction(transaction)
 
+      const tx = LibEIP155TxStruct(encodedTransaction)
       await expect(
-        OVM_ECDSAContractAccount.execute(encodedTransaction, {
+        OVM_ECDSAContractAccount.execute(tx, {
           gasLimit: 40000000,
         })
       ).to.be.revertedWith('Gas is not sufficient to execute the transaction.')
@@ -122,16 +125,19 @@ describe('OVM_ECDSAContractAccount', () => {
 
       Mock__OVM_ETH.smocked.transfer.will.return.with(false)
 
-      await expect(
-        OVM_ECDSAContractAccount.execute(encodedTransaction)
-      ).to.be.revertedWith('Fee was not transferred to relayer.')
+      const tx = LibEIP155TxStruct(encodedTransaction)
+      await expect(OVM_ECDSAContractAccount.execute(tx)).to.be.revertedWith(
+        'Fee was not transferred to relayer.'
+      )
     })
 
     it(`should transfer value if value is greater than 0`, async () => {
       const transaction = { ...DEFAULT_EIP155_TX, value: 1234, data: '0x' }
       const encodedTransaction = await wallet.signTransaction(transaction)
 
-      await OVM_ECDSAContractAccount.execute(encodedTransaction)
+      await OVM_ECDSAContractAccount.execute(
+        LibEIP155TxStruct(encodedTransaction)
+      )
 
       // First call transfers fee, second transfers value (since value > 0).
       expect(
@@ -155,7 +161,7 @@ describe('OVM_ECDSAContractAccount', () => {
       })
 
       await expect(
-        OVM_ECDSAContractAccount.execute(encodedTransaction)
+        OVM_ECDSAContractAccount.execute(LibEIP155TxStruct(encodedTransaction))
       ).to.be.revertedWith('Value could not be transferred to recipient.')
     })
 
@@ -164,7 +170,7 @@ describe('OVM_ECDSAContractAccount', () => {
       const encodedTransaction = await wallet.signTransaction(transaction)
 
       await expect(
-        OVM_ECDSAContractAccount.execute(encodedTransaction)
+        OVM_ECDSAContractAccount.execute(LibEIP155TxStruct(encodedTransaction))
       ).to.be.revertedWith('Value transfer in contract creation not supported.')
     })
 
@@ -173,7 +179,7 @@ describe('OVM_ECDSAContractAccount', () => {
       const encodedTransaction = await wallet.signTransaction(transaction)
 
       await expect(
-        OVM_ECDSAContractAccount.execute(encodedTransaction)
+        OVM_ECDSAContractAccount.execute(LibEIP155TxStruct(encodedTransaction))
       ).to.be.revertedWith('Value is nonzero but input data was provided.')
     })
 
@@ -184,7 +190,7 @@ describe('OVM_ECDSAContractAccount', () => {
       const encodedTransaction = await wallet.signTransaction(transaction)
 
       await expect(
-        OVM_ECDSAContractAccount.execute(encodedTransaction)
+        OVM_ECDSAContractAccount.execute(LibEIP155TxStruct(encodedTransaction))
       ).to.be.revertedWith(
         'Calls to self are disabled until upgradability is re-enabled.'
       )

packages/contracts/test/contracts/OVM/accounts/OVM_ProxyEOA.spec.ts

@@ -1,18 +1,22 @@
 import { expect } from '../../../setup'
 
 /* External Imports */
-import { ethers } from 'hardhat'
-import { ContractFactory, Contract, Signer } from 'ethers'
+import { ethers, waffle } from 'hardhat'
+import { ContractFactory, Contract, Signer, Wallet } from 'ethers'
 import { MockContract, smockit } from '@eth-optimism/smock'
 import { toPlainObject } from 'lodash'
 
 /* Internal Imports */
 import { getContractInterface, predeploys } from '../../../../src'
+import { DEFAULT_EIP155_TX, LibEIP155TxStruct } from '../../../helpers'
 
 describe('OVM_ProxyEOA', () => {
   let signer: Signer
+  let wallet: Wallet
   before(async () => {
     ;[signer] = await ethers.getSigners()
+    const provider = waffle.provider
+    ;[wallet] = provider.getWallets()
   })
 
   let Mock__OVM_ExecutionManager: MockContract
@@ -67,21 +71,31 @@ describe('OVM_ProxyEOA', () => {
 
   describe('fallback()', () => {
     it(`should call delegateCall with right calldata`, async () => {
+      const transaction = { ...DEFAULT_EIP155_TX }
+      const encodedTransaction = await wallet.signTransaction(transaction)
+
       const data = Mock__OVM_ECDSAContractAccount.interface.encodeFunctionData(
         'execute',
-        ['0x12341234']
+        [LibEIP155TxStruct(encodedTransaction)]
       )
 
       await signer.sendTransaction({
         to: OVM_ProxyEOA.address,
         data,
       })
 
-      expect(
-        toPlainObject(Mock__OVM_ECDSAContractAccount.smocked.execute.calls[0])
-      ).to.deep.include({
-        _encodedTransaction: '0x12341234',
-      })
+      const call = toPlainObject(
+        Mock__OVM_ECDSAContractAccount.smocked.execute.calls[0]
+      )
+      const _transaction = call._transaction
+
+      expect(_transaction[0]).to.deep.equal(transaction.nonce)
+      expect(_transaction.nonce).to.deep.equal(transaction.nonce)
+      expect(_transaction.gasPrice).to.deep.equal(transaction.gasPrice)
+      expect(_transaction.gasLimit).to.deep.equal(transaction.gasLimit)
+      expect(_transaction.to).to.deep.equal(transaction.to)
+      expect(_transaction.data).to.deep.equal(transaction.data)
+      expect(_transaction.isCreate).to.deep.equal(false)
     })
 
     it.skip(`should return data from fallback`, async () => {