Spearbit Audit: Address contracts issues

[jakekidd]

How do

Go to: https://github.com/spearbit-audits/connext/issues

Go down the priority list: Critical>High Risk>Medium Risk>Low Risk>Gas Optimizations>Informational

If something is related to Nomad contracts:

• Add the "Nomad Contracts" label and "Status: Acknowledged" label
• Move on, you can ignore this for now (James will pick it up)

If something is related to SponsorVault:

• Add the "SponsorVault" label
• Move on, we will reassess and work on those issues once the core path issues have been addressed

Doing a fix

• Make a PR to 1464 branch with fixes for the issue: https://github.com/connext/nxtp/pull/1466
• Once it's r4r: add the label "Fix Ready for Review" and "Status: Fixed"
• Comment it in this issue
• Feel free to tag auditors and give them a chance to review your PR if you're uncertain the fix is correct
• Once the PR is merged, remove the "Fix Ready for Review" label

Progress

Note: This excludes Nomad contracts and SponsorVault issues!

• [ ] Critical
• [x] High Risk
• [ ] Medium Risk
• [ ] Low Risk
• [ ] Gas Optimizations
• [ ] Informational

[jakekidd]

r4r https://github.com/connext/nxtp/pull/1536

[jakekidd]

Discussions

Native assets, remove them entirely? Only wrappers?

@jakekidd : what's tradeoff here?

If we don't handle native ETH, then the user has to do a separate call to wrap the ETH to wETH. Annoying bc:

• multiple steps
• costs more gas since it's 2 sep. calls instead of delegate call

This could be amended by making a separate contract that does the wrapping step first... but is that going to bug out the msg.sender?

Also, on the execute side, you have the unwrapping, which could potentially be more annoying, but let's see:

• if user has no execute calldata, they can just make their calldata to go to the wETH contract and unwrap wETH -> ETH... but then that call also has to make sure the ETH gets sent to the target address!! does that require a sep. contract?
• if user does have calldata, this sounds... more difficult

Portal repayment swap from mad assets <> adopted assets uses oracle from our stableswap AMM pool which can be manipulated

https://github.com/spearbit-audits/connext/issues/44 @LayneHaber : basically when we repay portals we have to swap mad <> adopted assets. this uses our pool to calculate the swap, which mean the price can be easily manipulated to extract value. the question is where is the best place to get these prices? theres a couple of these and im not sure the best oracle to use -- can you plug in arbitrary dexes to a twap-like feed? cc @wanglonghong

Executor can be freely looted of any stuck tokens

https://github.com/spearbit-audits/connext/issues/24 See discussion on Discord: https://discord.com/channels/992549151134982234/999407710388899860/999407943357313067

Use a constant denominator value for min amount received calculations using slippage tolerances.

Related to https://github.com/spearbit-audits/connext/issues/63 See https://github.com/connext/nxtp/pull/1536#issuecomment-1189634954 @jakekidd : Maybe for the min amount received calculations we should have a hardcoded denominator (as in, a constant value) that's separate from s.LIQUIDITY_FEE_DENOMINATOR? Would make this additional check in xcall significantly cheaper and avoid a bunch of redundant sloads where we're using it. It seems like it should be a constant value anyway as best practice (being able to change it is dangerous for frontends that don't check the current on-chain value).

[jakekidd]

Before issue is closed out, would like to make sure the following is addressed in documentation:

The two big conclusions from the audit in terms of policy/security that xapp developers need to know are:

• If you have calldata that manages funds on the destination chain, xapps need to be 100% sure that there is no possibility of funds (any funds, including the tokens that were bridged and any/all other tokens) getting accidentally left in the Executor contract. Otherwise, those funds are forfeit (they can technically be scooped up by just about anyone with very little effort).

• Given this new policy change: xapp developers using the callback pattern must be certain that it does not revert. Otherwise, the relayer will be paid and the callback will have been executed/completed (afaik, there aren't any "redos" here, lol).

For both items above ^ we can (and should) be demonstrating an example for each of a "bad pattern" and a safe pattern

[LayneHaber]

are we okay with closing this? remediation work is done, documentation likely needs to be updated still. should that be a separate issue?

[alexwhte]

yes closing

if we need heavy documentation work please file to backlog?