monorepo
monorepo copied to clipboard
connext
Internal audit review
Problem Need to set aside time for final reviews by code owners on contracts prior to audit.
Acceptance Criteria
- [ ] @jakekidd does a full review
- [ ] @liu-zhipeng does a full review
- [ ] @LayneHaber does a full review
Impact
- What OKR goal does this align to?
Shipping amarok to audit
- Provide Impact estimate = ideally quantitative forecast, if not then qualitative description of impact
7
Other Notes A clear and concise description of what the problem is
There could be missing simplifications, optimizations, or other lingering code improvements within the contracts. The core implementation team should take time to review the implementation with this in mind
Opened an internal-audit branch. Please base your PRs off of that branch!
List any PRs for the purpose of internal audit here!
#2076 this got done as part of internal audit https://github.com/connext/nxtp/pull/2084
- domain indexer and queue still need forge unit tests
working on that on branch
lib-message-tests, which also includes a fix for: https://hackmd.io/@-HV50kYcRqOjl_7du8m1AA/HJbJOh6Go#Finding-MerklebranchRoot-can-attribute-a-leaf-to-its-root-at-multiple-indices
- "iff delay block == 0, just skip the whole validation queue" needs an issue opened
- looks like we are doing a redundant verification period check with the mainnet spoke connector
(separate from above issue)
- delay block needs for validation period technically could vary on a chain by chain basis. rollups, for example, don't even need verification period (for one of the directions, either to L2 or from L2, can't remember). hub chain receives inbound roots from all spoke chains. we could have mapping for delay blocks per chain on hub?? or we could make it so validation period requirements can be disabled (i.e. delay blocks = 0) for certain peripheral chains? hmmm (separate from above issue)
two separate issues that are likely both out of scope. the first one is technically more pressing, but both are harmless sec-wise as far as I can tell, only are unideal gas-cost-wise:
-
let's say for some reason, we call sendMessage twice on a spoke domain and send the same outbound root twice. they will both arrive and both be accepted into the queue cause, like, nothing stopping the same exact root getting inserted twice. it's not like we check to make sure we haven't received this message root before.
-
if you send an outbound root from a spoke, it includes all of the message hashes so far (ofc). if we send another outbound root later, it will include new message hashes. if there's 2 different outbound roots in the hub queue from the same spoke and they both have been verified and one is earlier and the other is later, we might as well just send the later one! currently, we send both.
- inconsistent use of interface IERC20 in bridgefacet: noting this bc we literally use SafeERC20 for IERC20 under libraries in this contract