Skip to content

test(integration): withdrawing a fake L2 token#2061

Merged
tynes merged 1 commit intodevelopfrom
m/add-fake-token-test
Feb 1, 2022
Merged

test(integration): withdrawing a fake L2 token#2061
tynes merged 1 commit intodevelopfrom
m/add-fake-token-test

Conversation

@maurelian
Copy link
Copy Markdown
Contributor

@maurelian maurelian commented Jan 24, 2022 •
edited by tynes
Loading

Description

Adds an integration test demonstrating that a commonly perceived vulnerability does not in fact exist.

Specifically, there appears to an obvious bug which would allow an attacker to withdraw a fake ERC20 token from L2 in exchange for a real ERC20 (such as WBTC) token on L1. There is no check in the L2StandardBridge, however the withdrawal is prevented from finalizing by a check in the L1StandardBridge.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jan 24, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 0293749

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/integration-tests Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the A-integration Area: integration tests label Jan 24, 2022
@maurelian maurelian closed this Jan 24, 2022
@maurelian maurelian force-pushed the m/add-fake-token-test branch from 7305517 to eb926e2 Compare January 24, 2022 15:53
@github-actions github-actions bot removed the A-integration Area: integration tests label Jan 24, 2022
@maurelian maurelian reopened this Jan 24, 2022
@github-actions github-actions bot added the A-integration Area: integration tests label Jan 24, 2022
@maurelian maurelian marked this pull request as ready for review January 24, 2022 16:15
@maurelian maurelian requested a review from mslipper January 24, 2022 16:46
tynes
tynes reviewed Jan 24, 2022
View reviewed changes
tynes
tynes reviewed Jan 24, 2022
View reviewed changes
tynes
tynes reviewed Jan 24, 2022
View reviewed changes
@tynes
Copy link
Copy Markdown
Contributor

tynes commented Jan 24, 2022

Can you add a changeset for the integration tests?

@maurelian maurelian marked this pull request as draft January 24, 2022 19:38
@maurelian maurelian force-pushed the m/add-fake-token-test branch 3 times, most recently from d302a2a to 71ddfa7 Compare January 24, 2022 20:40
maurelian
maurelian commented Jan 24, 2022
View reviewed changes
integration-tests/test/bridged-tokens.spec.ts
'0x'
)
await env.relayXDomainMessages(withdrawalTx)
await env.waitForXDomainTransaction(withdrawalTx, Direction.L2ToL1)
Copy link
Copy Markdown
Contributor Author

@maurelian maurelian Jan 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally I would assert that this transaction does not emit an ERC20WithdrawalFinalized event, but I did not see a logs field on the remoteTx object which this returns.

It's been a while since I've worked with the cross-domain testing utils, am I missing something?

@maurelian maurelian marked this pull request as ready for review January 24, 2022 20:44
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 24, 2022 •
edited
Loading

Codecov Report

Merging #2061 (71ddfa7) into develop (eb926e2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2061   +/-   ##
========================================
  Coverage    74.58%   74.58%           
========================================
  Files           79       79           
  Lines         2554     2554           
  Branches       401      401           
========================================
  Hits          1905     1905           
  Misses         649      649
Flag Coverage Δ
batch-submitter 62.50% <ø> (ø)
contracts 90.48% <ø> (ø)
core-utils 57.50% <ø> (ø)
data-transport-layer 38.64% <ø> (ø)
message-relayer 70.86% <ø> (ø)
sdk 88.38% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core-utils/src/bn.ts 100.00% <0.00%> (ø)
packages/core-utils/src/fees.ts 52.94% <0.00%> (ø)
packages/core-utils/src/alias.ts 81.81% <0.00%> (ø)
packages/sdk/src/utils/coercion.ts 88.46% <0.00%> (ø)
packages/sdk/src/utils/contracts.ts 93.33% <0.00%> (ø)
packages/sdk/src/cross-chain-provider.ts 82.67% <0.00%> (ø)
packages/sdk/src/utils/message-encoding.ts 100.00% <0.00%> (ø)
packages/core-utils/src/common/test-utils.ts 95.55% <0.00%> (ø)
packages/core-utils/src/coders/sequencer-batch.ts 100.00% <0.00%> (ø)
...kages/data-transport-layer/src/utils/validation.ts 10.71% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb926e2...71ddfa7. Read the comment docs.

@maurelian maurelian force-pushed the m/add-fake-token-test branch from 71ddfa7 to 0293749 Compare January 25, 2022 15:24
@maurelian maurelian requested review from elenadimitrova and removed request for mslipper January 25, 2022 17:56
@maurelian
Copy link
Copy Markdown
Contributor Author

maurelian commented Jan 25, 2022

@elenadimitrova I've requested your review as this test implements the bridge withdrawal bug which we've received several false positive reports on.

@tynes tynes merged commit 137f776 into develop Feb 1, 2022
@tynes tynes deleted the m/add-fake-token-test branch February 1, 2022 17:44
theochap pushed a commit that referenced this pull request Dec 10, 2025
@refcell @clabby
feat(p2p): Peer Protection (#2061)
6d0290d 
### Description

Implements peer protection rpc handling.

Closes #1573
Closes #1574

---------

Co-authored-by: clabby <ben@clab.by>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tynes tynes tynes approved these changes

+1 more reviewer

@elenadimitrova elenadimitrova elenadimitrova approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-integration Area: integration tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maurelian @tynes @codecov-commenter @elenadimitrova