Cross-chain Strategy

[shahthepro]

Code Changes

Check readme: https://github.com/OriginProtocol/origin-dollar/blob/shah/cross-chain-strategy-cctpv2/contracts/contracts/strategies/crosschain/crosschain-strategy.md

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.81%. Comparing base (c1b85d1) to head (4ebe400).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...strategies/crosschain/CrossChainMasterStrategy.sol	84.33%	13 Missing ⚠️
...strategies/crosschain/CrossChainRemoteStrategy.sol	86.02%	12 Missing and 1 partial ⚠️
...s/strategies/crosschain/AbstractCCTPIntegrator.sol	90.47%	9 Missing and 1 partial ⚠️
...s/contracts/strategies/Generalized4626Strategy.sol	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2715      +/-   ##
==========================================
+ Coverage   39.40%   43.81%   +4.40%     
==========================================
  Files         126      133       +7     
  Lines        5789     6131     +342     
  Branches     1537     1636      +99     
==========================================
+ Hits         2281     2686     +405     
+ Misses       3506     3441      -65     
- Partials        2        4       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sparrowDom]

"- the Mainnet part" should probably be "- the L2 chain part"

[sparrowDom]

Idea how to change message finality checks

[sparrowDom]

Latest batch of review comments

[sparrowDom]

I know we emit a more generic "MessageTransmitted" here. It might be useful to also emit a separate BalanceCheck event here.

[shahthepro]

Any reason why this would be necessary? As far as our offchain component is concerned it's not gonna care about the payload, it only needs to know if there's a pending transfer for it to poll CCTP API for attestation and relay it

[shahthepro]

Also, if we add this here, we will need to emit this whenever we send the balance check as an acknowledgement as well to be consistent. It's not a big deal but I don't see any particular reason why this would be necessary

[sparrowDom]

I was thinking that it might be useful for an offchain analytics / Grafana to detect if a balanceUpdate messages are being triggered as expected and that the earnings from the Remote to Master are being reported.

[sparrowDom]

Posting a couple more comments

[sparrowDom]

WIP - publishing partial review

Requirements

Pull request implements a strategy with a novel concept where the strategy contract is split into 2 parts each part residing on a different chain. The 2 strategy contracts use CCTP for USDC token transfer and message relaying between the 2 strategy components. One contract is called the master strategy and the other the remote strategy. Master strategy is the one receiving and sending funds from the vault. The remote strategy is responsible for deploying and withdrawing funds to the underlying platform.

Easy Checks

Authentication

• Never use tx.origin
• Every external/public function is supposed to be externally accessible
• Every external/public function has the correct authentication
• All initializers have onlyGovernor
• Each method that changes access control has the correct access control

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.
• Contract is not vulnerable to being sent self destruct ETH
• If contract interacts with ETH make sure there are no read only reentrancies (like this one in Curve pools)

Cryptographic code

• This contract code does not roll it's own crypto.
• No signature checks without reverting on a 0x00 result.
• No signed data could be used in a replay attack, on our contract or others.

Gas problems

• Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.
  • no loops

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying. If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

• Code is solidity version >= 0.8.0
• All for loops use uint256 no loops

License

• The contract uses the appropriate limited BUSL-1.1 (Business) or the open MIT license

Proxy

• No storage variable initialized at definition when contract used as a proxy implementation.
• Any added storage slots are after existing slots.
• Any added inheritance does not affect storage slots for upgradable contracts.

Events

• All state changing functions emit events

Medium Checks

Rounding and casts

• Contract rounds in the protocols favor
• Contract does not have bugs from loosing rounding precision
• Code correctly multiplies before division
• Contract does not have bugs from zero or near zero amounts
• Safecast is aways used when casting no casting

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
• If OpenZeppelin ACL roles are use review & enumerate all of them.
• Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

External calls

• [] Contract addresses passed in are validated
  • 🟠 a couple of addresses not yet validated
• No unsafe external calls
• Reentrancy guards on all state changing functions
  • 🟠 some are missing guards
  • Still doesn't protect against external contracts changing the state of the world if they are called.
• No malicious behaviors
• Low level call() must require success.
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Tests

• Each publicly callable method has a test
• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested
• If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing.

Deploy

• Deployer permissions are removed after deploy

Strategy Specific

Remove this section if the code being reviewed is not a strategy.

Strategy checks

• Check balance cannot be manipulated up AND down by an attacker
• No read only reentrancy on downstream protocols during checkBalance
  • 🟡 hard to check depending on where the Yearn team will deposit funds to
• All reward tokens are collected
  • ⚪ rewards are not handled by the strategy contract
• The harvester can sell all reward tokens
  • ⚪ this will be done by a specialized multisig
• No funds are left in the contract that should not be as a result of depositing or withdrawing
• If the strategy deals with staking LP tokens any liquidity altering function: deposit, depositAll, withdraw, withdrawAll or custom (e.g. rebalance) should result in a state where all LP tokens owned by the contract remain staked
• All funds can be recovered from the strategy by some combination of depositAll, withdraw, or withdrawAll()
• WithdrawAll() can always withdraw an amount equal to or larger than checkBalances report, even in spite of attacker manipulation.
  • ⚪ there are no attackers to Morpho Vault, since our strategy contract will be the only one deploying funds
• WithdrawAll() cannot be MEV'd
• [] WithdrawAll() does not revert when strategy has 0 assets
  • 🟠 WithdrawAll can revert if the strategy has a deposit or withdrawal in flight
• Strategist cannot steal funds

Downstream

• We have monitoring on all backend protocol's governances
• We have monitoring on a pauses in all downstream systems

Thinking

Logic

Are there bugs in the logic?

• Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).

Deployment Considerations

Are there things that must be done on deploy, or in the wider ecosystem for this code to work. Are they done?

Resource usage

• Identify if the contract interacts with any external contracts and alters their state. If there is an expectation of the external contract's state confirm that any/all operations meet it.

Internal State

• What can be always said about relationships between stored state
• What must hold true about state before a function can run correctly (preconditions)
• What must hold true about the return or any changes to state after a function has run.

Does this code do that?

Attack

What could the impacts of code failure in this code be.

What conditions could cause this code to fail if they were not true.

Does this code successfully block all attacks.

Flavor

Could this code be simpler?

Could this code be less vulnerable to other code behaving weirdly?