Skip to content

fix(ctb): Address DoS vulnerability in OptimismPortal's proveWithdrawalTransaction#4025

Closed
clabby wants to merge 8 commits intodevelopfrom
@clabby/ctb/fix-withdrawal-dos
Closed

fix(ctb): Address DoS vulnerability in OptimismPortal's proveWithdrawalTransaction#4025
clabby wants to merge 8 commits intodevelopfrom
@clabby/ctb/fix-withdrawal-dos

Conversation

@clabby
Copy link
Contributor

@clabby clabby commented Nov 21, 2022

Overview

Fixes a high severity vulnerability in OptimismPortal that was introduced in #3836.

Prior to this fix, a malicious party could censor a withdrawal by repeatedly proving it, causing the 7 day timer to reset each time. The solution is to not allow any withdrawalHash to be proven more than once unless the withdrawal is re-proven with a different outputRoot / outputRootProof.

Tests

Added:

  • test_proveWithdrawalTransaction_replayProve_reverts
  • test_proveWithdrawalTransaction_replayProveChangedOutputRoot_success

Additional context

See November 2022 TOB Audit Week 1 report.

@changeset-bot
Copy link

changeset-bot bot commented Nov 21, 2022

🦋 Changeset detected

Latest commit: 9d06292

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

This PR includes changesets to release 5 packages
Name Type
@eth-optimism/contracts-bedrock Patch
@eth-optimism/actor-tests Patch
@eth-optimism/sdk Patch
@eth-optimism/drippie-mon Patch
@eth-optimism/message-relayer 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 2-reviewers A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Nov 21, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 21, 2022

Hey @clabby! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Nov 21, 2022
@mergify mergify bot removed the conflict label Nov 21, 2022
@clabby clabby changed the title fix(ctb): Don't allow for replaying withdrawal proofs in OptimismPortal fix(ctb): Address DoS vulnerability in OptimismPortal's proveWithdrawalTransaction Nov 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2022

Codecov Report

Merging #4025 (9f02838) into develop (ee7abf3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4025   +/-   ##
========================================
  Coverage    43.59%   43.60%           
========================================
  Files          328      328           
  Lines        17156    17158    +2     
  Branches       781      782    +1     
========================================
+ Hits          7480     7482    +2     
- Misses        9171     9172    +1     
+ Partials       505      504    -1     
Flag Coverage Δ
bedrock-go-tests 37.94% <ø> (ø)
contracts-bedrock-tests 38.51% <100.00%> (+0.17%) ⬆️
contracts-governance-tests 83.05% <ø> (ø)
contracts-periphery-tests 96.49% <ø> (ø)
contracts-tests 98.92% <ø> (ø)
core-utils-tests 60.63% <ø> (ø)
dtl-tests 47.15% <ø> (ø)
fault-detector-tests 34.18% <ø> (ø)
sdk-tests 40.01% <ø> (ø)

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

Impacted Files Coverage Δ
.../contracts-bedrock/contracts/L1/OptimismPortal.sol 94.73% <100.00%> (+0.29%) ⬆️
op-node/heartbeat/service.go 54.05% <0.00%> (-10.82%) ⬇️
op-node/sources/batching.go 81.25% <0.00%> (-4.47%) ⬇️
op-node/p2p/discovery.go 70.10% <0.00%> (+3.09%) ⬆️

Copy link
Contributor

@maurelian maurelian left a comment

Choose a reason for hiding this comment

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

Left some minor suggestions but generally LGTM

@mergify
Copy link
Contributor

mergify bot commented Nov 22, 2022

Hey @clabby! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Nov 22, 2022
@mergify mergify bot removed the conflict label Nov 22, 2022
@clabby
Copy link
Contributor Author

clabby commented Nov 23, 2022

@smartcontracts @maurelian The *-docker-build actions don't seem to like the branch name here- re-opened under #4057 with a passing CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-pkg-contracts-bedrock Area: packages/contracts-bedrock

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants