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

fix(wallet): reduce scope of address checks in useTransactionParser hook #11300

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

onyb
Copy link
Member

@onyb onyb commented Nov 26, 2021

Summary of changes:

  • Reintroduced same address error checks (removed in Allow self-transfers in panel for cancel transactions #11231), and restricted its scope to exclude ETHSend, which is the type of cancel transactions.
  • Restricted the scope of the error check for recipient addresses being known contracts, to only include ERC20Transfer, ERC721TransferFrom, and ERC721SafeTransferFrom. The error in the linked issue was because ETH-WETH swap didn't use the 0x exchange router but actually the WETH contract directly.
  • In ERC721 transfers, sender may not be the same as the owner. The same address check is now exclusively performed between between owner and recipient. For example, we now allow an account to transfer ownership of an ERC721 to itself.
  • Added an extensive test suite.

Resolves brave/brave-browser#19745.

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Demo

Before

ETH-WETH.swap.error.mov

After

WETH-ETH Swap Cancel transaction
ETH-WETH.swap.mov
cancel.mov

Unit tests

@onyb onyb self-assigned this Nov 26, 2021
@github-actions github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Nov 26, 2021
@onyb onyb requested a review from a team November 26, 2021 07:23
@onyb onyb force-pushed the f/wallet/tx-parser-errors branch 2 times, most recently from 04d432d to 47f7157 Compare November 26, 2021 07:31
@onyb
Copy link
Member Author

onyb commented Nov 26, 2021

CI failure on ios seems unrelated => failed to apply patches.

Copy link
Contributor

@Douglashdaniel Douglashdaniel left a comment

Choose a reason for hiding this comment

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

Looks good!
Great job on the Tests!
Should we have a Security Review for @testing-library/react-hooks?

@onyb
Copy link
Member Author

onyb commented Nov 26, 2021

@Douglashdaniel Yes, added an issue for the security review.

@bbondy
Copy link
Member

bbondy commented Nov 26, 2021

I think the dep is ok but checking with @diracdeltas since it's new for:
"@testing-library/react-hooks"

Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

lgtm

@onyb
Copy link
Member Author

onyb commented Nov 29, 2021

Merging after green light by security team.

@srirambv
Copy link
Contributor

Verification passed on

Brave 1.34.46 Chromium: 96.0.4664.45 (Official Build) nightly (x86_64 translated)
Revision 76e4c1bb2ab4671b8beba3444e61c0f17584b2fc-refs/branch-heads/4664@{#947}
OS macOS Version 12.0.1 (Build 21A559)
  • Verified able to submit a ETH-WETH transaction without getting the token's contract address error message
11300.-.ETH-WETH.Swap.mov
  • Verified able to cancel a transaction and confirm it. Cancelled transaction doesn't show up on block explorer but the cancelling transaction does get confirmed on the block
11300.-.Cancel.Transaction.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"The receiving address is a tokens contract address" should only be applied to ERC20 and NFT transfers.
4 participants