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 settling when one side closes/updates with outdated BalanceProof #1689

Merged
merged 6 commits into from
Jun 12, 2020

Conversation

andrevmatos
Copy link
Contributor

Fixes #1607

Short description
Search on transfer history for matching EnvelopeMessage (if needed) and use its values to settle channel.

Definition of Done

  • Steps to manually test the change have been documented
  • Acceptance criteria are met
  • Change has been manually tested by the reviewer (dApp)

Steps to manually test the change (dApp)

  1. This condition won't happen naturally on real life, each end usually going to close/settle with their latest view of partner's BalanceProof, in order to get the most tokens, but an attacker could specifically chose to close with outdated BP in order to prevent us from settling the channel.
  2. Therefore, specially tweaked tests were made to trigger this condition and ensure we still can find the right BP and settle nonetheless.

@andrevmatos andrevmatos added sdk 🖥 security Pull requests that address a security vulnerability labels Jun 11, 2020
@andrevmatos andrevmatos requested a review from kelsos June 11, 2020 23:59
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #1689 into master will decrease coverage by 1.58%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1689      +/-   ##
==========================================
- Coverage   96.85%   95.27%   -1.59%     
==========================================
  Files          64      148      +84     
  Lines        3688     5222    +1534     
  Branches      829      957     +128     
==========================================
+ Hits         3572     4975    +1403     
- Misses        105      195      +90     
- Partials       11       52      +41     
Flag Coverage Δ
#dapp 91.46% <ø> (?)
#sdk 96.81% <100.00%> (-0.04%) ⬇️
#sdk_integration ?
Impacted Files Coverage Δ
raiden-ts/src/transfers/actions.ts 100.00% <ø> (ø)
raiden-ts/src/transfers/reducer.ts 96.46% <ø> (-0.18%) ⬇️
raiden-ts/src/channels/epics.ts 98.02% <100.00%> (+0.77%) ⬆️
raiden-ts/src/channels/reducer.ts 96.93% <100.00%> (ø)
raiden-ts/src/channels/types.ts 100.00% <100.00%> (ø)
raiden-ts/src/messages/utils.ts 99.00% <100.00%> (ø)
raiden-ts/src/migration/3.ts 100.00% <100.00%> (ø)
raiden-ts/src/services/epics.ts 99.03% <100.00%> (ø)
raiden-ts/src/transfers/utils.ts 93.33% <100.00%> (+1.53%) ⬆️
raiden-ts/src/polyfills.ts 85.71% <0.00%> (-14.29%) ⬇️
... and 86 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 c7514d1...38a3820. Read the comment docs.

Copy link
Contributor

@kelsos kelsos left a comment

Choose a reason for hiding this comment

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

Lgtm 👍

@andrevmatos andrevmatos merged commit 9161ce4 into master Jun 12, 2020
@andrevmatos andrevmatos deleted the fix/settle_outdated branch June 12, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdk 🖥 security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDK can't settle if partner closes with outdated balanceHash
2 participants