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

Add erc1271 support to contract account #1052

Merged
merged 9 commits into from
Jun 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/blue-cooks-join.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@eth-optimism/integration-tests': patch
'@eth-optimism/contracts': patch
---

Adds ERC1271 support to default contract account
49 changes: 49 additions & 0 deletions integration-tests/test/predeploys.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { expect } from 'chai'
import { ethers } from 'hardhat'

/* Imports: External */
import { Contract, Wallet } from 'ethers'
import { OptimismEnv } from './shared/env'
import { DEFAULT_TRANSACTION } from './shared/utils'
import { getContractInterface } from '@eth-optimism/contracts'

describe('ECDSAContractAccount', () => {
let l2Wallet: Wallet

before(async () => {
const env = await OptimismEnv.new()
l2Wallet = env.l2Wallet
})

let ProxyEOA: Contract
let messageHash: string
let signature: string

before(async () => {
// Send a transaction to ensure there is a ProxyEOA deployed at l2Wallet.address
const result = await l2Wallet.sendTransaction(DEFAULT_TRANSACTION)
await result.wait()
ProxyEOA = new Contract(
l2Wallet.address,
getContractInterface('OVM_ECDSAContractAccount'),
l2Wallet
)
const message = '0x42'
messageHash = ethers.utils.hashMessage(message)
signature = await l2Wallet.signMessage(message)
})

it('should correctly evaluate isValidSignature from this wallet', async () => {
const isValid = await ProxyEOA.isValidSignature(messageHash, signature)
expect(isValid).to.equal('0x1626ba7e')
})

it('should correctly evaluate isValidSignature from other wallet', async () => {
const otherWallet = Wallet.createRandom().connect(l2Wallet.provider)
const isValid = await ProxyEOA.connect(otherWallet).isValidSignature(
messageHash,
signature
)
expect(isValid).to.equal('0x1626ba7e')
})
})
15 changes: 6 additions & 9 deletions integration-tests/test/rpc.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@ import {
import { Wallet, BigNumber, Contract, ContractFactory } from 'ethers'
import { ethers } from 'hardhat'
import chai, { expect } from 'chai'
import { sleep, l2Provider, l1Provider, fundUser } from './shared/utils'
import {
sleep,
l2Provider,
DEFAULT_TRANSACTION,
fundUser,
} from './shared/utils'
import chaiAsPromised from 'chai-as-promised'
import { OptimismEnv } from './shared/env'
import {
Expand All @@ -22,14 +27,6 @@ describe('Basic RPC tests', () => {
let env: OptimismEnv
let wallet: Wallet

const DEFAULT_TRANSACTION = {
to: '0x' + '1234'.repeat(10),
gasLimit: 33600000000001,
gasPrice: 0,
data: '0x',
value: 0,
}

const provider = injectL2Context(l2Provider)

let Reverter: Contract
Expand Down
8 changes: 8 additions & 0 deletions integration-tests/test/shared/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,11 @@ const abiCoder = new utils.AbiCoder()
export const encodeSolidityRevertMessage = (_reason: string): string => {
return '0x08c379a0' + remove0x(abiCoder.encode(['string'], [_reason]))
}

export const DEFAULT_TRANSACTION = {
to: '0x' + '1234'.repeat(10),
gasLimit: 33600000000001,
gasPrice: 0,
data: '0x',
value: 0,
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { OVM_ETH } from "../predeploys/OVM_ETH.sol";

/* External Imports */
import { SafeMath } from "@openzeppelin/contracts/math/SafeMath.sol";
import { ECDSA } from "@openzeppelin/contracts/cryptography/ECDSA.sol";
K-Ho marked this conversation as resolved.
Show resolved Hide resolved

/**
* @title OVM_ECDSAContractAccount
Expand Down Expand Up @@ -57,6 +58,26 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
return;
}

/**
* @dev Should return whether the signature provided is valid for the provided data
* @param hash Hash of the data to be signed
* @param signature Signature byte array associated with _data
*/
function isValidSignature(
bytes32 hash,
bytes memory signature
)
public
view
returns (
bytes4 magicValue
)
{
return ECDSA.recover(hash, signature) == address(this) ?
this.isValidSignature.selector :
bytes4(0);
}

/**
* Executes a signed transaction.
* @param _transaction Signed EIP155 transaction.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { expect } from '../../../setup'

/* External Imports */
import { ethers, waffle } from 'hardhat'
import { ContractFactory, Contract, Wallet, BigNumber } from 'ethers'
import { ContractFactory, Contract, Wallet, BigNumber, utils } from 'ethers'
import { MockContract, smockit } from '@eth-optimism/smock'
import { toPlainObject } from 'lodash'

Expand Down Expand Up @@ -187,7 +187,10 @@ describe('OVM_ECDSAContractAccount', () => {
// NOTE: Upgrades are disabled for now but will be re-enabled at a later point in time. See
// comment in OVM_ECDSAContractAccount.sol for additional information.
it(`should revert if trying call itself`, async () => {
const transaction = { ...DEFAULT_EIP155_TX, to: wallet.address }
const transaction = {
...DEFAULT_EIP155_TX,
to: wallet.address,
}
const encodedTransaction = await wallet.signTransaction(transaction)

await expect(
Expand All @@ -197,4 +200,25 @@ describe('OVM_ECDSAContractAccount', () => {
)
})
})

describe('isValidSignature()', () => {
const message = '0x42'
const messageHash = ethers.utils.hashMessage(message)
it(`should revert for a malformed signature`, async () => {
await expect(
OVM_ECDSAContractAccount.isValidSignature(messageHash, '0xdeadbeef')
).to.be.revertedWith('ECDSA: invalid signature length')
})

it(`should return 0 for an invalid signature`, async () => {
const signature = await wallet.signMessage(message)
const bytes = await OVM_ECDSAContractAccount.isValidSignature(
messageHash,
signature
)
expect(bytes).to.equal('0x00000000')
})
// NOTE: There is no good way to unit test verifying a valid signature
// An integration test exists testing this instead
Comment on lines +221 to +222
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smartcontracts set up the Lib_ExecutionManagerWrapper.ovmADDRESS to execute in a separate delegated call frame so that it can be smocked, see "smocked.ovmADDRESS" elsewhere in these tests. This is why we otherwise use ovmADDRESS in the contract account instead of address(this). Alternatively in this case we could use await smockit('OVM_ECDSAContractAccount', {address: wallet.address}) to make the address(this) work in this case IIRC. Either way, I think the integration test we have is probably fine for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't use Lib_ExecutionManagerWrapper.ovmADDRESS because we need to keep isValidSignature as a view function. Going to try out that smockit approach

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:/ smockit didn't work for this, going to just keep my integration test

})
K-Ho marked this conversation as resolved.
Show resolved Hide resolved
})