Commit 8a15b68f authored by Mark Tyneway's avatar Mark Tyneway

contracts-bedrock: update systemconfig resource config validation

Updates the way that the system config validates the resource
config to ensure that its not possible to set config values
that break the system. It is possible to set config values
that would burn too much gas, rendering the system unusable.
Every error cannot be caught, so try to catch the most possible.
Altering the default resource config is not recommended, but it
is possible.

This also adds fuzz tests that cover the problem, if the change
in the require statement in the system config contract is reverted,
then the fuzz tests catch the `UNDEFINED` error quite quickly.
parent 8e355d1d
This diff is collapsed.
This diff is collapsed.
...@@ -80,7 +80,7 @@ contract SystemConfig is OwnableUpgradeable, Semver { ...@@ -80,7 +80,7 @@ contract SystemConfig is OwnableUpgradeable, Semver {
event ConfigUpdate(uint256 indexed version, UpdateType indexed updateType, bytes data); event ConfigUpdate(uint256 indexed version, UpdateType indexed updateType, bytes data);
/** /**
* @custom:semver 1.2.0 * @custom:semver 1.3.0
* *
* @param _owner Initial owner of the contract. * @param _owner Initial owner of the contract.
* @param _overhead Initial overhead value. * @param _overhead Initial overhead value.
...@@ -98,7 +98,7 @@ contract SystemConfig is OwnableUpgradeable, Semver { ...@@ -98,7 +98,7 @@ contract SystemConfig is OwnableUpgradeable, Semver {
uint64 _gasLimit, uint64 _gasLimit,
address _unsafeBlockSigner, address _unsafeBlockSigner,
ResourceMetering.ResourceConfig memory _config ResourceMetering.ResourceConfig memory _config
) Semver(1, 2, 0) { ) Semver(1, 3, 0) {
initialize({ initialize({
_owner: _owner, _owner: _owner,
_overhead: _overhead, _overhead: _overhead,
...@@ -270,7 +270,7 @@ contract SystemConfig is OwnableUpgradeable, Semver { ...@@ -270,7 +270,7 @@ contract SystemConfig is OwnableUpgradeable, Semver {
"SystemConfig: min base fee must be less than max base" "SystemConfig: min base fee must be less than max base"
); );
// Base fee change denominator must be greater than 0. // Base fee change denominator must be greater than 0.
require(_config.baseFeeMaxChangeDenominator > 0, "SystemConfig: denominator cannot be 0"); require(_config.baseFeeMaxChangeDenominator > 1, "SystemConfig: denominator must be larger than 1");
// Max resource limit plus system tx gas must be less than or equal to the L2 gas limit. // Max resource limit plus system tx gas must be less than or equal to the L2 gas limit.
// The gas limit must be increased before these values can be increased. // The gas limit must be increased before these values can be increased.
require( require(
......
...@@ -1085,3 +1085,88 @@ contract OptimismPortalUpgradeable_Test is Portal_Initializer { ...@@ -1085,3 +1085,88 @@ contract OptimismPortalUpgradeable_Test is Portal_Initializer {
assertEq(slot21Expected, slot21After); assertEq(slot21Expected, slot21After);
} }
} }
/**
* @title OptimismPortalResourceFuzz_Test
* @dev Test various values of the resource metering config to ensure that deposits cannot be
* broken by changing the config.
*/
contract OptimismPortalResourceFuzz_Test is Portal_Initializer {
/**
* @dev The max gas limit observed throughout this test. Setting this too high can cause
* the test to take too long to run.
*/
uint256 constant MAX_GAS_LIMIT = 30_000_000;
/**
* @dev Test that various values of the resource metering config will not break deposits.
*/
function testFuzz_systemConfigDeposit_succeeds(
uint32 _maxResourceLimit,
uint8 _elasticityMultiplier,
uint8 _baseFeeMaxChangeDenominator,
uint32 _minimumBaseFee,
uint32 _systemTxMaxGas,
uint128 _maximumBaseFee,
uint64 _gasLimit,
uint64 _prevBoughtGas,
uint128 _prevBaseFee,
uint8 _blockDiff
) external {
// Get the set system gas limit
uint64 gasLimit = systemConfig.gasLimit();
// Bound resource config
_maxResourceLimit = uint32(bound(_maxResourceLimit, 21000, MAX_GAS_LIMIT / 8));
_gasLimit = uint64(bound( _gasLimit, 21000, _maxResourceLimit));
_prevBaseFee = uint128(bound(_prevBaseFee, 0, 5 gwei));
// Prevent values that would cause reverts
vm.assume(gasLimit >= _gasLimit);
vm.assume(_minimumBaseFee < _maximumBaseFee);
vm.assume(_baseFeeMaxChangeDenominator > 1);
vm.assume(uint256(_maxResourceLimit) + uint256(_systemTxMaxGas) <= gasLimit);
vm.assume(_elasticityMultiplier > 0);
vm.assume(
((_maxResourceLimit / _elasticityMultiplier) * _elasticityMultiplier) == _maxResourceLimit
);
_prevBoughtGas = uint64(bound(_prevBoughtGas, 0, _maxResourceLimit - _gasLimit));
_blockDiff = uint8(bound(_blockDiff, 0, 3));
// Create a resource config to mock the call to the system config with
ResourceMetering.ResourceConfig memory rcfg = ResourceMetering.ResourceConfig({
maxResourceLimit: _maxResourceLimit,
elasticityMultiplier: _elasticityMultiplier,
baseFeeMaxChangeDenominator: _baseFeeMaxChangeDenominator,
minimumBaseFee: _minimumBaseFee,
systemTxMaxGas: _systemTxMaxGas,
maximumBaseFee: _maximumBaseFee
});
vm.mockCall(
address(systemConfig),
abi.encodeWithSelector(systemConfig.resourceConfig.selector),
abi.encode(rcfg)
);
// Set the resource params
uint256 _prevBlockNum = block.number - _blockDiff;
vm.store(
address(op),
bytes32(uint256(1)),
bytes32((_prevBlockNum << 192) | (uint256(_prevBoughtGas) << 128) | _prevBaseFee)
);
// Ensure that the storage setting is correct
(uint128 prevBaseFee, uint64 prevBoughtGas, uint64 prevBlockNum) = op.params();
assertEq(prevBaseFee, _prevBaseFee);
assertEq(prevBoughtGas, _prevBoughtGas);
assertEq(prevBlockNum, _prevBlockNum);
// Do a deposit, should not revert
op.depositTransaction{ gas: MAX_GAS_LIMIT }({
_to: address(0x20),
_value: 0x40,
_gasLimit: _gasLimit,
_isCreation: false,
_data: hex""
});
}
}
...@@ -110,7 +110,7 @@ contract SystemConfig_Setters_TestFail is SystemConfig_Init { ...@@ -110,7 +110,7 @@ contract SystemConfig_Setters_TestFail is SystemConfig_Init {
maximumBaseFee: 2 gwei maximumBaseFee: 2 gwei
}); });
vm.prank(sysConf.owner()); vm.prank(sysConf.owner());
vm.expectRevert("SystemConfig: denominator cannot be 0"); vm.expectRevert("SystemConfig: denominator must be larger than 1");
sysConf.setResourceConfig(config); sysConf.setResourceConfig(config);
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment