Skip to content

Commit 82f0179

Browse files
committed
Address CR comments
1 parent f20d464 commit 82f0179

9 files changed

Lines changed: 59 additions & 83 deletions

File tree

contracts/contracts/strategies/BridgedWOETHMigrationStrategy.sol

Lines changed: 23 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@ import { Client } from "@chainlink/contracts-ccip/src/v0.8/ccip/libraries/Client
99

1010
import { BridgedWOETHStrategy } from "./BridgedWOETHStrategy.sol";
1111
import { IStrategy } from "../interfaces/IStrategy.sol";
12-
import { IVault } from "../interfaces/IVault.sol";
13-
import { NativeFeeHelper } from "./crosschainV3/libraries/NativeFeeHelper.sol";
1412
import { CCIPMessageBuilder } from "./crosschainV3/libraries/CCIPMessageBuilder.sol";
1513

1614
/**
@@ -22,16 +20,15 @@ import { CCIPMessageBuilder } from "./crosschainV3/libraries/CCIPMessageBuilder.
2220
* retaining V1's local deposit/withdraw + oracle pipeline (inherited unchanged).
2321
*
2422
* Storage carries forward V1's two existing fields (lastOraclePrice, maxPriceDiffBps)
25-
* and appends three new ones (totalBridged, maxPerBridge, operator) plus an upgrade
23+
* and appends two new ones (totalBridged, maxPerBridge) plus an upgrade
2624
* gap. All cross-chain configuration that doesn't change between deploys lives in
2725
* immutables: `master` is both the local Master strategy on Base (read for
2826
* in-flight reconciliation) and the cross-chain CCIP recipient on Ethereum (same
2927
* address by CreateX-driven parity).
3028
*
3129
* Access pattern:
32-
* - `bridgeToRemote` callable by operator, governor, or strategist.
30+
* - `bridgeToRemote` callable by governor or strategist.
3331
* - `setMaxPerBridge` callable by governor or strategist.
34-
* - `setOperator` callable by governor only.
3532
* - V1's `setMaxPriceDiffBps` (governor-only) and depositBridgedWOETH /
3633
* withdrawBridgedWOETH (governor or strategist) are inherited unchanged.
3734
*/
@@ -59,15 +56,11 @@ contract BridgedWOETHMigrationStrategy is BridgedWOETHStrategy {
5956
/// @notice Per-call cap on `bridgeToRemote`, configurable by governor or strategist.
6057
uint256 public maxPerBridge;
6158

62-
/// @notice Automation EOA permitted to drive `bridgeToRemote` calls.
63-
address public operator;
64-
65-
uint256[47] private __gap;
59+
uint256[48] private __gap;
6660

6761
// --- Events -----------------------------------------------------------
6862

6963
event MaxPerBridgeSet(uint256 maxPerBridge);
70-
event OperatorUpdated(address oldOperator, address newOperator);
7164
event WOETHBridgedToRemote(uint256 amount, uint256 totalBridged);
7265

7366
// --- Errors -----------------------------------------------------------
@@ -101,24 +94,7 @@ contract BridgedWOETHMigrationStrategy is BridgedWOETHStrategy {
10194
ccipChainSelectorMainnet = _ccipChainSelectorMainnet;
10295
}
10396

104-
// --- Access control ---------------------------------------------------
105-
106-
modifier onlyOperatorGovernorOrStrategist() {
107-
require(
108-
msg.sender == operator ||
109-
isGovernor() ||
110-
msg.sender == IVault(vaultAddress).strategistAddr(),
111-
"BWM: not authorised"
112-
);
113-
_;
114-
}
115-
116-
// --- Operator / cap configuration ------------------------------------
117-
118-
function setOperator(address _operator) external onlyGovernor {
119-
emit OperatorUpdated(operator, _operator);
120-
operator = _operator;
121-
}
97+
// --- Cap configuration ------------------------------------------------
12298

12399
function setMaxPerBridge(uint256 _maxPerBridge)
124100
external
@@ -147,7 +123,7 @@ contract BridgedWOETHMigrationStrategy is BridgedWOETHStrategy {
147123
function bridgeToRemote(uint256 _amount)
148124
external
149125
payable
150-
onlyOperatorGovernorOrStrategist
126+
onlyGovernorOrStrategist
151127
nonReentrant
152128
{
153129
require(_amount > 0 && _amount <= maxPerBridge, "BWM: bad amount");
@@ -169,7 +145,7 @@ contract BridgedWOETHMigrationStrategy is BridgedWOETHStrategy {
169145
);
170146

171147
uint256 fee = ccipRouter.getFee(ccipChainSelectorMainnet, ccipMessage);
172-
NativeFeeHelper.consume(fee);
148+
_consumeNativeFee(fee);
173149

174150
IERC20(address(bridgedWOETH)).safeApprove(address(ccipRouter), _amount);
175151
ccipRouter.ccipSend{ value: fee }(
@@ -183,6 +159,23 @@ contract BridgedWOETHMigrationStrategy is BridgedWOETHStrategy {
183159

184160
receive() external payable {}
185161

162+
/// @dev Consume `fee` in native for the CCIP send. Two sources:
163+
/// - `msg.value == 0` → pre-funded: `address(this).balance` covers the fee.
164+
/// - `msg.value > 0` → user-paid: excess refunds to `msg.sender`.
165+
/// Reverts when the chosen source doesn't cover `fee`.
166+
function _consumeNativeFee(uint256 fee) internal {
167+
if (msg.value == 0) {
168+
require(address(this).balance >= fee, "Fee: unfunded");
169+
return;
170+
}
171+
require(msg.value >= fee, "Fee: insufficient");
172+
if (msg.value > fee) {
173+
// slither-disable-next-line low-level-calls
174+
(bool ok, ) = msg.sender.call{ value: msg.value - fee }("");
175+
require(ok, "Fee: refund failed");
176+
}
177+
}
178+
186179
// --- checkBalance override (WETH-only accounting) --------------------
187180

188181
/**

contracts/contracts/strategies/crosschainV3/DESIGN.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -457,7 +457,9 @@ The manual burn-relay in `relay()` works on both V2.0 and V2.1. But
457457
`CCTPAdapter._quoteFee` calls `tokenMessenger.getMinFeeAmount(amount)`,
458458
which is V2.1-only. If a chain has only V2.0 deployed, `quoteFee(amount > 0)`
459459
reverts. Current deploys (OETHb) don't use CCTP at all, so this is a
460-
non-issue. OUSD V3 spoke chains must be on V2.1 — check before deploying.
460+
non-issue. OUSD V3 spoke chains must be on V2.1 — check before deploying
461+
against Circle's per-chain contract list:
462+
https://developers.circle.com/stablecoins/evm-smart-contracts
461463

462464
### 4.4 Lost claim-ack stalls `pendingWithdrawalAmount`
463465

contracts/contracts/strategies/crosschainV3/MasterWOTokenStrategy.sol

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ contract MasterWOTokenStrategy is AbstractWOTokenStrategy {
5959

6060
event RemoteStrategyBalanceUpdated(uint256 yieldBaseline);
6161
event DepositRequested(uint64 nonce, uint256 amount);
62+
event DepositSkipped(uint256 amount, uint256 minTransfer);
6263
event DepositAcked(uint64 nonce, uint256 yieldBaseline);
6364
event WithdrawRequested(uint64 nonce, uint256 amount);
6465
event WithdrawRequestAcked(uint64 nonce, uint256 yieldBaseline);
@@ -332,11 +333,17 @@ contract MasterWOTokenStrategy is AbstractWOTokenStrategy {
332333
// transfers it before calling `deposit`) and stays counted in `checkBalance`, so it
333334
// auto-deposits once enough accumulates. A revert here would DoS the mint ->
334335
// `_allocate` -> `deposit` path. Covers both `deposit` and `depositAll`.
335-
if (_amount < IBridgeAdapter(outboundAdapter).minTransferAmount()) {
336+
uint256 minTransfer = IBridgeAdapter(outboundAdapter)
337+
.minTransferAmount();
338+
if (_amount < minTransfer) {
339+
// Signal the skipped deposit so off-chain monitoring can see the amount stayed
340+
// local (still counted in `checkBalance`) instead of being silently dropped.
341+
emit DepositSkipped(_amount, minTransfer);
336342
return;
337343
}
338344

339345
uint64 nonce = _getNextYieldNonce();
346+
// Store the pending deposit amount until the remote deposit is acknowledged.
340347
pendingDepositAmount = _amount;
341348

342349
// bridgeAsset → outboundAdapter allowance is granted once in `_setOutboundAdapter`.
@@ -390,6 +397,7 @@ contract MasterWOTokenStrategy is AbstractWOTokenStrategy {
390397
);
391398

392399
uint64 nonce = _getNextYieldNonce();
400+
// Store the pending withdrawal amount until the remote withdrawal is claimed.
393401
pendingWithdrawalAmount = _amount;
394402

395403
bytes memory payload = CrossChainV3Helper.encodeUint256(_amount);
@@ -540,6 +548,7 @@ contract MasterWOTokenStrategy is AbstractWOTokenStrategy {
540548
function _processDepositAck(uint64 nonce, bytes memory payload) internal {
541549
_markYieldNonceProcessed(nonce);
542550
uint256 yieldBaseline = CrossChainV3Helper.decodeUint256(payload);
551+
// Store the acknowledged deposit in the remote balance and clear the pending amount.
543552
remoteStrategyBalance = yieldBaseline;
544553
pendingDepositAmount = 0;
545554
emit DepositAcked(nonce, yieldBaseline);

contracts/contracts/strategies/crosschainV3/README.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ contracts/strategies/crosschainV3/adapters/
4242
contracts/strategies/crosschainV3/libraries/
4343
CCTPMessageHelper.sol — CCTP V2 wire-format decoder: transport header + burn-message body
4444
CCIPMessageBuilder.sol — shared CCIP `Client.EVM2AnyMessage` construction
45-
NativeFeeHelper.sol — shared native-fee consumption helper
4645
4746
contracts/proxies/create2/
4847
CrossChainStrategyProxy.sol — Master/Remote strategy proxy (CreateX/CREATE2-deployable for peer parity)

contracts/contracts/strategies/crosschainV3/RemoteWOTokenStrategy.sol

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -447,12 +447,11 @@ contract RemoteWOTokenStrategy is AbstractWOTokenStrategy {
447447
}
448448

449449
function _opportunisticClaim() internal {
450-
uint256 stored = outstandingRequestId;
451-
if (stored == REQUEST_ID_EMPTY) {
450+
// `outstandingRequestId` stores the vault id verbatim.
451+
uint256 vaultRequestId = outstandingRequestId;
452+
if (vaultRequestId == REQUEST_ID_EMPTY) {
452453
return;
453454
}
454-
// `outstandingRequestId` stores the vault id verbatim.
455-
uint256 vaultRequestId = stored;
456455
// Hoist `claimed` outside the try so its scope is unambiguous to static
457456
// analysers (avoids the slither uninitialized-local false-positive that
458457
// fired when `claimed` was named only in the try-returns clause).

contracts/contracts/strategies/crosschainV3/adapters/CCIPAdapter.sol

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,11 @@ contract CCIPAdapter is AbstractAdapter, IAny2EVMMessageReceiver, IERC165 {
3737
ccipRouter = _ccipRouter;
3838
}
3939

40+
/// @dev Inbound liveness: if the CCIP DON never auto-executes a delivered message, anyone can
41+
/// trigger manual execution. That path still runs `OffRamp.manuallyExecute` ->
42+
/// `Router.routeMessage` -> `ccipReceive`, so `msg.sender` is the Router here even on a
43+
/// manual replay and this guard passes — no governance escape hatch is needed.
44+
/// https://docs.chain.link/ccip/tutorials/evm/manual-execution#trigger-manual-execution
4045
modifier onlyRouter() {
4146
require(msg.sender == address(ccipRouter), "CCIP: not router");
4247
_;

contracts/contracts/strategies/crosschainV3/libraries/NativeFeeHelper.sol

Lines changed: 0 additions & 34 deletions
This file was deleted.

contracts/deploy/base/102_oethb_v3_woeth_v2_upgrade.js

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,13 @@ module.exports = deployOnBase(
6767
args: [dMigrationImpl.address],
6868
},
6969
// 2. Set the per-call cap. (Governor-or-strategist gate; runs as governance here.)
70+
// `bridgeToRemote` is governor-or-strategist gated, so the strategist can drive it
71+
// directly — no separate operator authorisation needed.
7072
{
7173
contract: cMigration,
7274
signature: "setMaxPerBridge(uint256)",
7375
args: [MAX_PER_BRIDGE],
7476
},
75-
// 3. Authorise the multichain strategist as the operator for `bridgeToRemote`.
76-
{
77-
contract: cMigration,
78-
signature: "setOperator(address)",
79-
args: [addresses.multichainStrategist],
80-
},
8177
],
8278
};
8379
}

contracts/test/strategies/crosschainV3/transfer-caps.js

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,10 @@ describe("Unit: Adapter transfer caps", function () {
525525
await bridgeAsset.mintTo(master.address, ONE_K.div(2)); // 500
526526
await outbound.setMinTransferAmountOverride(ONE_K); // floor 1000
527527

528-
await mockL2Vault.callDepositAll(master.address);
528+
// Signals the skip so it isn't silently dropped.
529+
await expect(mockL2Vault.callDepositAll(master.address))
530+
.to.emit(master, "DepositSkipped")
531+
.withArgs(ONE_K.div(2), ONE_K);
529532

530533
// Nothing bridged; funds stay local and are still counted in checkBalance.
531534
expect(await outbound.lastAmountSent()).to.equal(0);
@@ -542,11 +545,15 @@ describe("Unit: Adapter transfer caps", function () {
542545
await bridgeAsset.mintTo(master.address, ONE_K.div(2)); // 500
543546
await outbound.setMinTransferAmountOverride(ONE_K); // floor 1000
544547

545-
await mockL2Vault.callDeposit(
546-
master.address,
547-
bridgeAsset.address,
548-
ONE_K.div(2)
549-
);
548+
await expect(
549+
mockL2Vault.callDeposit(
550+
master.address,
551+
bridgeAsset.address,
552+
ONE_K.div(2)
553+
)
554+
)
555+
.to.emit(master, "DepositSkipped")
556+
.withArgs(ONE_K.div(2), ONE_K);
550557

551558
expect(await master.pendingDepositAmount()).to.equal(0);
552559
expect(await bridgeAsset.balanceOf(master.address)).to.equal(

0 commit comments

Comments
 (0)