04.25 Omniscia Audit Remediation

[nksazonov]

Context

As the NitroLite project continue evolving, this audit was adamant to highlight the flaws present so far.

During the audit process itself the project required to change directions, leading to drastic changes being introduced.
This has resulted into an Audit covering an older version of the protocol, therefore fixes are going to be applied to the commit 74986b8, while many of them will be cherry-picked on master afterwards.

It should also be noted that after an audit process was complete, it became obvious that some of the files will be drastically changed, henceforth we decided not to spend resources on fixing related issues.

Results

All changes to the codebase since the Audit commit (74986b8) can be tracked on a feat/04-25-audit branch. ALL changes are available in 6c00d29 commit.

Fell out of scope

We ACKNOWLEDGE all of the following issues WITHOUT creating a fix for them, as the contracts are obsolete and will be DRASTICALLY CHANGED soon.

• CSU-01C: Inefficient Conditionals (informational)
• NRP-01C: Misleading Documentation (informational)
• CSU-01M: Insufficient Adjudicator Implementation (major)
• CRE-01M: Insufficient Adjudicator Implementation (major)
• MPT-01M: Inexplicable Micropayment Channel Adjudicator (medium)
• MPT-02M: Insufficient Adjudicator Implementation (major)

Acknowledged

We simply acknowledge the following issues without comments:

• CYD-01C: Ineffectual Usage of Safe Arithmetics (informational)
• CYD-05C: Non-Standard Usage of Library (informational)

Not relevant / remedied by the protocol

We believe that the following issues are not relevant to the protocol itself, are already resolved by protocol intentions or rules, or it is a 3rd party being responsible for avoiding such an issue.

• [CYD-06M] Manipulation of Finalized Deposit Distribution #70 (minor) - see comment
• [CYD-11M] Insufficient Validation of Adjudicator #71 (medium) - see comment
• [CYD-12M] Insufficient Validation of Duplicate Channel ID Creation #57 (medium) - see comment
• [CYD-14M] Potential Race Condition of Channel ID Reservation #39 (medium) - see comment
• [CYD-15M] Flawed Adjudication Concept #56 (major) - see comment

Remedied

We have amended changes to the codebase, hopefully resolving the following issues:

Manual Review:

• [CYD-01M] Inexplicable Extra Function Argument #67 (unknown) - fixed by [CYD-02M] Duplicate Participant Incompatibility #68
• [CYD-02M] Duplicate Participant Incompatibility #68 (minor) - fixed by fix(Custody::create): addresses checks #69
• [CYD-03M] Improper EIP-20 Transfer Evaluations #46 (minor) - fixed by fix(Custody): use SafeERC20 for ERC20 transfers #53 , fix: cyd 03m 06c full alleviation #77
• [CYD-04M] Inexistent Conformance to Checks-Effects-Interactions Pattern #45 (minor) - fixed by fix(Custody::withdraw): conform to CEI pattern #52
• [CYD-05M] Inexistent Prevention of Native Fund Loss #44 (minor) - fixed by fix(Custody::deposit): check value=0 if deposit ERC20 #50
• [CYD-07M] Checkpoint / Challenge Race Condition #58 (medium) - fixed by fix(Custody::challenge): add challenger signature #64 + comment
• [CYD-08M] Improper Introduction of Channel #43 (medium) - fixed by fix(Custody): add chan to participant ledger #49
• [CYD-09M] Incorrect Join Requirement Evaluation #42 (medium) - fixed by feat(Custody): restrict to 2 participant channels #63
• [CYD-10M] Inexistent Introduction of Channel for First Participant #41 (medium) - fixed by fix(Custody): add chan to participant ledger #49
• [CYD-13M] Insufficient Validation of Stage #40 (medium) - fixed by feat(Custody): update checkpoint and challenge #55
• [CYD-16M] Incorrect Fund Distribution on Channel Closure #38 (major) - fixed by feat(Custody): remove account locked tracking #48

Code Style:

• CYD-02C: Inefficient Increment Operation
  Fixed by feat(Custody): remove account locked tracking #48.

• CYD-03C: Inefficient Loop Limit Evaluation
  Fixed by feat(Custody): restrict to 2 participant channels #63.

Fixed by #66:

• CYD-04C: Inefficient mapping Lookups
• CYD-06C: Redundant Local Variables
• CYD-07C: Redundant Parenthesis Statements
• CYD-08C: Redundant Restriction
• CYD-09C: Suboptimal Struct Declaration Styles
• USL-01C: Non-Standard Usage of Library
• DYM-01C: Redundant Named Arguments

[dimast-x]

@nksazonov Thanks. Is this still relevant? Please close the issue if it is now outdated.