Skip to content

Commit f62faa2

Browse files
authored
Audit fix: CGT vs Lockbox. (#418)
* Audit fix: CGT vs Lockbox. * Import cleanup.
1 parent 55317e3 commit f62faa2

5 files changed

Lines changed: 58 additions & 2 deletions

File tree

packages/contracts-bedrock/src/L1/OPContractsManagerStandardValidator.sol

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,12 @@ contract OPContractsManagerStandardValidator is ISemver {
316316
_errors = internalRequire(_sysCfg.operatorFeeConstant() == 0, "SYSCON-120", _errors);
317317
_errors = internalRequire(getProxyAdmin(address(_sysCfg)) == _admin, "SYSCON-130", _errors);
318318
_errors = internalRequire(_sysCfg.superchainConfig() == superchainConfig, "SYSCON-140", _errors);
319+
320+
// CGT prevents Lockbox
321+
if (_sysCfg.isCustomGasToken()) {
322+
_errors = internalRequire(!_sysCfg.isFeatureEnabled(Features.ETH_LOCKBOX), "SYSCON-150", _errors);
323+
}
324+
319325
return _errors;
320326
}
321327

@@ -493,6 +499,8 @@ contract OPContractsManagerStandardValidator is ISemver {
493499
_errors = internalRequire(getProxyAdmin(address(_lockbox)) == _admin, "LOCKBOX-30", _errors);
494500
_errors = internalRequire(_lockbox.systemConfig() == _sysCfg, "LOCKBOX-40", _errors);
495501
_errors = internalRequire(_lockbox.authorizedPortals(_portal), "LOCKBOX-50", _errors);
502+
// Lockbox is not compatible with CGT
503+
_errors = internalRequire(!_sysCfg.isCustomGasToken(), "LOCKBOX-60", _errors);
496504
return _errors;
497505
}
498506

packages/contracts-bedrock/src/L1/OptimismPortal2.sol

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -806,7 +806,9 @@ contract OptimismPortal2 is Initializable, ResourceMetering, ReinitializableBase
806806
/// @notice Checks if the ETHLockbox feature is enabled.
807807
/// @return bool True if the ETHLockbox feature is enabled.
808808
function _isUsingLockbox() internal view returns (bool) {
809-
return systemConfig.isFeatureEnabled(Features.ETH_LOCKBOX) && address(ethLockbox) != address(0);
809+
// CGT prevents Lockbox
810+
(address token,) = gasPayingToken();
811+
return token == Constants.ETHER && systemConfig.isFeatureEnabled(Features.ETH_LOCKBOX) && address(ethLockbox) != address(0);
810812
}
811813

812814
/// @notice Asserts that the contract is not paused.

packages/contracts-bedrock/src/L1/SystemConfig.sol

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import { GasPayingToken, IGasToken } from "src/libraries/GasPayingToken.sol";
1515

1616
// Interfaces
1717
import { ISemver } from "interfaces/universal/ISemver.sol";
18-
import { IOptimismPortal2 } from "interfaces/L1/IOptimismPortal2.sol";
1918
import { IResourceMetering } from "interfaces/L1/IResourceMetering.sol";
2019
import { IOptimismPortal2 } from "interfaces/L1/IOptimismPortal2.sol";
2120
import { ISuperchainConfig } from "interfaces/L1/ISuperchainConfig.sol";
@@ -561,6 +560,11 @@ contract SystemConfig is ProxyAdminOwnedBase, OwnableUpgradeable, Reinitializabl
561560
// Features can only be set by the ProxyAdmin or its owner.
562561
_assertOnlyProxyAdminOrProxyAdminOwner();
563562

563+
// Prevent Lockbox on CGT chains
564+
if (_enabled && _feature == Features.ETH_LOCKBOX && isCustomGasToken()) {
565+
revert SystemConfig_InvalidFeatureState();
566+
}
567+
564568
// As a sanity check, prevent users from enabling the feature if already enabled or
565569
// disabling the feature if already disabled. This helps to prevent accidental misuse.
566570
if ((_enabled && isFeatureEnabled[_feature]) || (!_enabled && !isFeatureEnabled[_feature])) {

packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3493,3 +3493,26 @@ contract OptimismPortal2WithMockERC20_Test is OptimismPortal2_FinalizeWithdrawal
34933493
optimismPortal2.depositERC20Transaction(address(0), 0, 0, 0, false, "");
34943494
}
34953495
}
3496+
3497+
/// @title OptimismPortal2_CGTLockboxGuard_Test
3498+
/// @notice Lockbox must stay inactive on CGT chains.
3499+
contract OptimismPortal2_CGTLockboxGuard_Test is OptimismPortal2_TestInit {
3500+
/// @notice Deposit on a CGT chain never calls lockETH, even with lockbox force-enabled.
3501+
function test_deposit_customGasToken_skipsLockbox_succeeds() external {
3502+
forceEnableLockbox(address(ethLockbox));
3503+
3504+
// Mock CGT
3505+
vm.mockCall(
3506+
address(systemConfig),
3507+
abi.encodeCall(systemConfig.gasPayingToken, ()),
3508+
abi.encode(address(42), uint8(18))
3509+
);
3510+
3511+
// lockETH must NOT be called
3512+
vm.expectCall(address(ethLockbox), abi.encodeCall(ethLockbox.lockETH, ()), 0);
3513+
3514+
// Deposit with msg.value = 0
3515+
vm.prank(alice, alice);
3516+
optimismPortal2.depositTransaction(address(0xbeef), 0, 100_000, false, "");
3517+
}
3518+
}

packages/contracts-bedrock/test/L1/SystemConfig.t.sol

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,6 +1066,25 @@ contract SystemConfig_SetFeature_Test is SystemConfig_TestInit {
10661066
}
10671067
}
10681068

1069+
/// @title SystemConfig_SetFeature_CustomGasToken_Test
1070+
/// @notice Validates that ETH_LOCKBOX cannot be enabled on a Custom Gas Token chain.
1071+
contract SystemConfig_SetFeature_CustomGasToken_Test is SystemConfig_Init_CustomGasToken {
1072+
/// @notice Enabling ETH_LOCKBOX on a CGT chain must revert.
1073+
function test_setFeature_ethLockboxOnCGTChain_reverts() external {
1074+
vm.prank(address(systemConfig.proxyAdmin()));
1075+
vm.expectRevert(ISystemConfig.SystemConfig_InvalidFeatureState.selector);
1076+
systemConfig.setFeature(Features.ETH_LOCKBOX, true);
1077+
}
1078+
1079+
/// @notice Non-lockbox features remain unaffected on CGT chains.
1080+
function test_setFeature_nonLockboxFeatureOnCGTChain_succeeds() external {
1081+
bytes32 otherFeature = "OTHER_FEATURE";
1082+
vm.prank(address(systemConfig.proxyAdmin()));
1083+
systemConfig.setFeature(otherFeature, true);
1084+
assertTrue(systemConfig.isFeatureEnabled(otherFeature));
1085+
}
1086+
}
1087+
10691088
/// @title SystemConfig_IsFeatureEnabled_Test
10701089
/// @notice Test contract for SystemConfig `isFeatureEnabled` function.
10711090
contract SystemConfig_IsFeatureEnabled_Test is SystemConfig_TestInit {

0 commit comments

Comments
 (0)