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

GalloDaSballo - tx.origin breaks ability to have someone else broadcast TXs and may cause loss of bonds #96

Closed
sherlock-admin3 opened this issue Apr 4, 2024 · 1 comment
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin3
Copy link

sherlock-admin3 commented Apr 4, 2024

GalloDaSballo

medium

tx.origin breaks ability to have someone else broadcast TXs and may cause loss of bonds

Summary

The use of tx.origin adds an additional risk to the bonding system that can be avoided by passing the payer in initialize

Vulnerability Detail

Impact

The FaultDisputeGame.initialize sets the starting claimData as follows:

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L555-L565

        claimData.push(
            ClaimData({
                parentIndex: type(uint32).max,
                counteredBy: address(0),
                claimant: tx.origin,
                bond: uint128(msg.value),
                claim: rootClaim(),
                position: ROOT_POSITION,
                clock: LibClock.wrap(Duration.wrap(0), Timestamp.wrap(uint64(block.timestamp)))
            })
        );

Setting the claimant to tx.origin this can be used to:

  • Steal a benign claimant bond by "sponsoring" their tx
  • Makes the system incompatible with Account Abstraction
  • Creates issues for Keepers (as the fee will be attributed to them instead of the actual payer)

The rest of the system doesn't prevent SCs or SC Wallets from interacting with it, but this setting does break composability

Code Snippet

  • User A wants to get their withdrawals proven automatically by a 3rd party service
  • The third party service will have their relayers EOAs steal funds and force them to work via a trusted system
  • The third party EOA will be tx.origin, user a will pay the bond and not be guaranteed to receive it back

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L559-L560

Tool used

Manual Review

Recommendation

A simple refactoring that forwards the original payer or specifying a bondReceiver would be sufficient in this case

Duplicate of #194

@github-actions github-actions bot added High A valid High severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Apr 11, 2024
@sherlock-admin2 sherlock-admin2 added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Apr 11, 2024
@nevillehuang nevillehuang removed the High A valid High severity issue label Apr 22, 2024
@sherlock-admin3 sherlock-admin3 changed the title Damaged Chartreuse Skunk - tx.origin breaks ability to have someone else broadcast TXs and may cause loss of bonds GalloDaSballo - tx.origin breaks ability to have someone else broadcast TXs and may cause loss of bonds Apr 23, 2024
@sherlock-admin3 sherlock-admin3 added Non-Reward This issue will not receive a payout and removed Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Apr 23, 2024
@Evert0x Evert0x added the Medium A valid Medium severity issue label May 15, 2024
@sherlock-admin2 sherlock-admin2 added Reward A payout will be made for this issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label and removed Non-Reward This issue will not receive a payout labels May 15, 2024
@sherlock-admin2
Copy link
Contributor

The protocol team fixed this issue in the following PRs/commits:
ethereum-optimism/optimism#10149

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

4 participants