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]

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

Pending PRs:

• address verification #2749
• naming #2748

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. Pr for this here
• No unsafe external calls
• Reentrancy guards on all state changing functions
  • 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
• Strategist cannot steal funds

Downstream

• We have monitoring on all backend protocol's governances
  • Yearn team will be managing the Morpho vault. There is no governance, as we have direct contact with them.
• We have monitoring on a pauses in all downstream systems
  • Morpho V2 Vaults specifically can not be paused

Thinking

Logic

The logic correctly handles the syncing of the strategies in respect of accounting for underlying balances, and messages / tokens being transferred via bridge. Also it considers that messages/tokens can have delayed arrival and can be delivered out of order.

• [] 🟠 Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).
• Here is the PR to correct the current inconsistencies:

Deployment Considerations

🟠 Yes we need to figure out how to use a shared (probably multisig deployer) utilising CreateX so that the Proxy addresses of our strategies are on the same address across chains. Also the multisig will deploy via a deployer salt protection, meaning no other account can deploy to the same addresses. We plan to do this during or after the audit

Resource usage

• Our contracts interacts with Morpho V2 Vault. The Generalized4626.sol contract interfacing with the Morpho V2 has some changes but none that would change the impact of how we've previously been integrating with the Morpho Vaults.

Internal State

The Master strategy must always correctly account for the shared funds between itself and the Remote strategy. Its accounting might be out of sync with the actual state, but it must eventually be correct. Any intermittent state should not over account or under account (except for accrued rewards) the total amount of funds. It is designed to be delayed, but not incorrect.

Attack

The 2 peer strategies trust the CCTP bridge when delivering messages and funds. It trusts that funds & message transfers happen in the CCTP documented procedure. Any deviation in this respect could cause unpredictable and potentially critical consequences. The strategy contracts don't have failsafes for such occurrences.

The strategy also trusts Yearn team will effectively manage the Morpho V2 Vault. And if any portion of the funds in the Vault is not liquid, the Yearn team will make them liquid upon our request. There are no on-chain assurances for this.

These are the risks with this strategy that can not be solved on-chain.

Flavor

Code is clean and effectively separates the CCTP communication part and the strategy logic part. This will be extra useful when we have other strategies implemented using CCTP as those will just reuse the CCTP bridge communication portion.