Add OUSD V3 and OETHb migration contracts#2909
Conversation
shahthepro
changed the title
shahthepro
requested review from
clement-ux,
naddison36 and
sparrowDom
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2909 +/- ##
==========================================
+ Coverage 44.63% 50.40% +5.76%
==========================================
Files 110 124 +14
Lines 4920 5815 +895
Branches 1362 1642 +280
==========================================
+ Hits 2196 2931 +735
- Misses 2721 2881 +160
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
naddison36
left a comment
naddison36
left a comment
There was a problem hiding this comment.
some nit comments on storage.
naddison36
left a comment
naddison36
left a comment
There was a problem hiding this comment.
Next round of comments
| function bridgeToRemote(uint256 _amount) | ||
| external | ||
| payable | ||
| onlyOperatorGovernorOrStrategist |
There was a problem hiding this comment.
this feels like something that only the Governor or Strategist should be doing.
I guess it depends on how much will be bridged each time. With 9,608 currently in the strategy, if the max is 1,000 ETH, then that's only 10 bridges which is reasonable to do with the Strategist.
If the max was 500 or less then I'd agree an operator EOA would be needed.
There was a problem hiding this comment.
The problem with having Governor or Strategist do it is that we would need enough capital and time to do it. We will have to borrow at least 1000 wOETH to bridge this in 9 cycles. With this upgrade to the strategy, we won't be needing that capital and we can just automate this with a script
There was a problem hiding this comment.
Why would changing bridgeToRemote to onlyGovernorOrStrategist need capital while using onlyOperatorGovernorOrStrategist does not need capital?
There was a problem hiding this comment.
Ah, sorry, I misread your comment. If we make this Guardian/strategist only, Talos relayer can't call this directly. So, we would either need another Safe module or trigger this call manually every hour or so
| address(bridgedWOETH), | ||
| _amount, | ||
| "", | ||
| master, |
There was a problem hiding this comment.
Isn't this meant to be the remove strategy on Ethereum?
There was a problem hiding this comment.
Both Remote and Master strategy are deployed using Create2 or Create3, so they should be on the same address on both chains
There was a problem hiding this comment.
ok, and we only have the immutable master variable in the contract.
Maybe add a comment that this is sending to the remote strategy which has the same address as the master strategy.
There was a problem hiding this comment.
Yep, added that comment yesterday in this commit: 8c529f2#diff-fd7d4905625ed21ee02a7cd4136ad7548b1d971c24d074129d912bffa359ae0c
| return localValueWETH; | ||
| } | ||
|
|
||
| uint256 bridgedValueWETH = (totalBridged * lastOraclePrice) / 1 ether; |
There was a problem hiding this comment.
checkBalance subtracts master.checkBalance(WETH) from the WETH value of all totalBridged. That assumes every WETH reported by Master corresponds to wOETH migrated out of this old strategy. If Master has any other balance component during the migration, such as local WETH, pending deposits, bridge adjustment, or value from another flow, the old strategy will underreport because that unrelated Master balance reduces this strategy’s in-flight amount. Is the migration guaranteeing no other Master activity/balance until the old strategy is removed?
There was a problem hiding this comment.
Yep, that's a good point. We discussed this and decided that we won't do any operation (deposit/withdrawal) while the migration is in progress. Realistically with a script to call the bridgeToRemote function every hour, it should take us roughly 9 hours to complete the migration. So it should be fine to pause all operations during that time
There was a problem hiding this comment.
That seems reasonable. We don't want to over engineer something that is only used once.
I was worried about a donation attacked of WETH but that doesn't seem to be an issue. Donating to BridgedWOETHMigrationStrategy doesn't change bridgedValueWETH and donating to the master strategy is also safe.
sparrowDom
left a comment
sparrowDom
left a comment
There was a problem hiding this comment.
Thanks for providing the fixes for previous comments. Some more comments inline:
| uint32 msgType, | ||
| uint256 amount, | ||
| bytes memory body | ||
| ) internal nonReentrant { |
There was a problem hiding this comment.
🟡 we don't have a test for Reentrency. This would be pretty convenient to have.
| // the fee; the pool is NOT consulted. Security gate: stops an | ||
| // attacker draining the operator-funded pool by spamming bridge | ||
| // in/out with msg.value = 0. | ||
| // userFunded = false → operator/protocol-funded sends (yield deposits/withdraws/claims |
There was a problem hiding this comment.
⚪ Would be cool to add a monitoring section to this PR where we should probably add a capability to Talos to observe the balances of fee assets on contracts. The remote contract sometimes needs to pay the bridging fee to convey the message. It can be blocked if it fails
| /// vault, which would be indistinguishable from "no request" if stored verbatim; | ||
| /// the +1 offset keeps `0` meaning "no outstanding (unclaimed) queue request" while | ||
| /// a real id of 0 is safely represented as 1. Cleared to 0 once the claim lands. | ||
| uint256 public outstandingRequestId; |
There was a problem hiding this comment.
nit: another way to approach this would be to set a const REQUEST_ID_EMPTY = type(uint256).max;
And set it to that whenever you want to treat it as empty
There was a problem hiding this comment.
That's a great suggestion, will do it.
| /// must equal the vault (enforced by the require below); Master always forwards the | ||
| /// received bridgeAsset to `vaultAddress` on the leg-2 ack. | ||
| /// | ||
| /// Only the `remoteStrategyBalance` slice is drawable here: `_amount` must be |
There was a problem hiding this comment.
nit: update this docs to reflect the changes where not only remoteStrategyBalance is taken into account
| Remote->>wOETH: deposit(OETH balance, Remote) | ||
| Remote->>wOETH: «OETH X» (wrapper pulls OETH on deposit) | ||
| wOETH-->>Remote: «wOETH shares» minted | ||
| Remote->>Remote: yieldBaseline = _viewCheckBalance() |
There was a problem hiding this comment.
_viewCheckBalance() − bridgeAdjustment
| Remote->>wOUSD: deposit(OUSD balance, Remote) | ||
| Remote->>wOUSD: «OUSD» (wrapper pulls on deposit) | ||
| wOUSD-->>Remote: «wOUSD shares» minted | ||
| Remote->>Remote: yieldBaseline = _viewCheckBalance() |
There was a problem hiding this comment.
_viewCheckBalance() − bridgeAdjustment
| Note over Remote: outstandingRequestId = requestId<br/>outstandingRequestAmount = amount | ||
|
|
||
| Note over Master,Remote: ─── Phase B: Remote sends WITHDRAW_REQUEST_ACK ─── | ||
| Remote->>Remote: yieldBaseline = _viewCheckBalance() |
There was a problem hiding this comment.
_viewCheckBalance() − bridgeAdjustment
| intermediate state; value lives in exactly one slot per row, and `checkBalance` | ||
| equals the total in every row. | ||
|
|
||
| | State | wOETH share value | OToken bal | bridgeAsset bal | queued\* | outstandingRequestId | checkBalance | |
There was a problem hiding this comment.
There are 2 new states: mint-failed; unwrap-ok/queue-fail);
| Master->>Master: _processWithdrawClaimAck success:<br/>_markYieldNonceProcessed(N+2)<br/>pendingWithdrawalAmount = 0<br/>remoteStrategyBalance = yieldBaseline | ||
| Master->>Vault: «WETH» transfer (forwards its full bridgeAsset balance) | ||
| Note over Master: safeTransfer(vaultAddress, balanceOf(this))<br/>emit Withdrawal(WETH, WETH, claimed) | ||
| else queue not yet matured (NACK) |
There was a problem hiding this comment.
Can revert for multiple reasons:
- outstandingRequestId != 0
- amount == 0
- bridgeAssetHeld < amount
- shipOutOfBounds
Moved the adapter participants close to the Bridge
naddison36
left a comment
naddison36
left a comment
There was a problem hiding this comment.
The next batch review comments
| } | ||
|
|
||
| function _opportunisticClaim() internal { | ||
| uint256 stored = outstandingRequestId; |
There was a problem hiding this comment.
nit: its not clear what store is later in the function code. I would use outstandingRequestIdMem instead
| return; | ||
| } | ||
| // `outstandingRequestId` stores the vault id verbatim. | ||
| uint256 vaultRequestId = stored; |
There was a problem hiding this comment.
Why create a new memory variable for what's stored in outstandingRequestId?
| // auto-deposits once enough accumulates. A revert here would DoS the mint -> | ||
| // `_allocate` -> `deposit` path. Covers both `deposit` and `depositAll`. | ||
| if (_amount < IBridgeAdapter(outboundAdapter).minTransferAmount()) { | ||
| return; |
There was a problem hiding this comment.
Feels like we should emit an event here since we are not doing the requested deposit while not failing the tx
| } | ||
|
|
||
| /// @inheritdoc IAny2EVMMessageReceiver | ||
| function ccipReceive(Client.Any2EVMMessage calldata message) |
There was a problem hiding this comment.
What guarantees, if any, does the CCIP Distributed Oracle Network (DON) provide for calling ccipReceive?
What if no DON participant calls ccipReceive? Can we retrigger the CCIP message?
From what I can tell, if the DON does not call ccipReceive then our strategy is stuck.
| } | ||
|
|
||
| uint64 nonce = _getNextYieldNonce(); | ||
| pendingDepositAmount = _amount; |
There was a problem hiding this comment.
nit: I like to make it clear when storage variables are writing back to storage. I'd add a comment like
// Store the pending deposit amount until the remote deposit is acknowledged.
| ); | ||
|
|
||
| uint64 nonce = _getNextYieldNonce(); | ||
| pendingWithdrawalAmount = _amount; |
There was a problem hiding this comment.
nit: I like to make it clear when storage variables are writing back to storage. I'd add a comment like
// Store the pending withdrawal amount until the remote withdrawal is claimed.
| function _processDepositAck(uint64 nonce, bytes memory payload) internal { | ||
| _markYieldNonceProcessed(nonce); | ||
| uint256 yieldBaseline = CrossChainV3Helper.decodeUint256(payload); | ||
| remoteStrategyBalance = yieldBaseline; |
There was a problem hiding this comment.
nit: I'd add a comment to make it clear this is a write to storage. eg
// Store the acknowledged deposit in the remote balance and clear the pending amount.
remoteStrategyBalance = yieldBaseline;
pendingDepositAmount = 0;
3 participants
Summary
OUSD V3 cross-chain strategy pair (Master/Remote) with bridge-agnostic adapter family (CCIP, CCTP V2, Superbridge), plus Sepolia ⇄ Base Sepolia testnet harness. Foundation for OETHb Phase 1 migration and future OUSD V3 L2 deployment rollouts.
Changes