Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Audit review fixes #106

Merged
merged 17 commits into from
Sep 6, 2023
Merged

Audit review fixes #106

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3648262
fix magic numbers and gas limit checks
dcbuild3r Sep 6, 2023
c3c2567
add setFxChild event and constructor input validation
dcbuild3r Sep 6, 2023
33df6b5
add setFxRootTunnel event and magic number documentation
dcbuild3r Sep 6, 2023
88beb2e
remove console import
dcbuild3r Sep 6, 2023
167b86b
fix deploy script logging messages
dcbuild3r Sep 6, 2023
13c401e
rename sendRoot to propagateRoot across contracts
dcbuild3r Sep 6, 2023
03a4871
add opStateBridge tests
dcbuild3r Sep 6, 2023
66c899a
add polygon state bridge and polygon world id tests
dcbuild3r Sep 6, 2023
399dec2
fix typos
dcbuild3r Sep 6, 2023
12d09df
set treeDepth to immutable and getter function to pure
dcbuild3r Sep 6, 2023
8e0e5b4
get treeDepth public
dcbuild3r Sep 6, 2023
d89e337
fix spelling emitted
dcbuild3r Sep 6, 2023
844da90
remove extra comment
dcbuild3r Sep 6, 2023
478a24e
add custom errors
dcbuild3r Sep 6, 2023
0c08228
more input validation
dcbuild3r Sep 6, 2023
b30336b
naming fixes
dcbuild3r Sep 6, 2023
fd56d7b
chore: fix natspec on GasLimitZero error
0xKitsune Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 46 additions & 11 deletions src/OpStateBridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ contract OpStateBridge is Ownable2Step {
/// EVENTS ///
///////////////////////////////////////////////////////////////////

/// @notice Emmitted when the the StateBridge gives ownership of the OPWorldID contract
/// @notice Emitted when the the StateBridge gives ownership of the OPWorldID contract
/// to the WorldID Identity Manager contract away
/// @param previousOwner The previous owner of the OPWorldID contract
/// @param newOwner The new owner of the OPWorldID contract
Expand All @@ -54,23 +54,23 @@ contract OpStateBridge is Ownable2Step {
address indexed previousOwner, address indexed newOwner, bool isLocal
);

/// @notice Emmitted when the the StateBridge sends a root to the OPWorldID contract
/// @notice Emitted when the the StateBridge sends a root to the OPWorldID contract
/// @param root The root sent to the OPWorldID contract on the OP Stack chain
event RootPropagated(uint256 root);

/// @notice Emmitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
/// @notice Emitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
/// @param rootHistoryExpiry The new root history expiry
event SetRootHistoryExpiry(uint256 rootHistoryExpiry);

/// @notice Emmitted when the the StateBridge sets the gas limit for sendRootOp
/// @notice Emitted when the the StateBridge sets the gas limit for sendRootOp
/// @param _opGasLimit The new opGasLimit for sendRootOp
event SetGasLimitPropagateRoot(uint32 _opGasLimit);

/// @notice Emmitted when the the StateBridge sets the gas limit for SetRootHistoryExpiry
/// @param _opGasLimit The new opGasLimit for SetRootHistoryExpiryt
/// @notice Emitted when the the StateBridge sets the gas limit for SetRootHistoryExpiry
/// @param _opGasLimit The new opGasLimit for SetRootHistoryExpiry
event SetGasLimitSetRootHistoryExpiry(uint32 _opGasLimit);

/// @notice Emmitted when the the StateBridge sets the gas limit for transferOwnershipOp
/// @notice Emitted when the the StateBridge sets the gas limit for transferOwnershipOp
/// @param _opGasLimit The new opGasLimit for transferOwnershipOptimism
event SetGasLimitTransferOwnershipOp(uint32 _opGasLimit);

Expand All @@ -81,6 +81,21 @@ contract OpStateBridge is Ownable2Step {
/// @notice Emitted when an attempt is made to renounce ownership.
error CannotRenounceOwnership();

/// @notice Emitted when an attempt is made to set the gas limit to zero or less
0xKitsune marked this conversation as resolved.
Show resolved Hide resolved
error GasLimitNotGreaterThanZero();

/// @notice Emitted when an attempt is made to set the owner to the zero address
error CannotSetZeroOwner();

/// @notice Emitted when an attempt is made to set the World ID Identity Manager to the zero address
error CannotSetWorldIDToZero();

/// @notice Emitted when an attempt is made to set the Op World ID Identity Manager to the zero address
error CannotSetOpWorldIDToZero();

/// @notice Emitted when an attempt is made to set the Cross Domain Messenger to the zero address
error CannotSetCrossDomainMessengerToZero();

dcbuild3r marked this conversation as resolved.
Show resolved Hide resolved
///////////////////////////////////////////////////////////////////
/// CONSTRUCTOR ///
///////////////////////////////////////////////////////////////////
Expand All @@ -95,6 +110,18 @@ contract OpStateBridge is Ownable2Step {
address _opWorldIDAddress,
address _crossDomainMessenger
) {
if (address(_worldIDIdentityManager) == address(0)) {
revert CannotSetWorldIDToZero();
}

if (address(_opWorldIDAddress) == address(0)) {
revert CannotSetOpWorldIDToZero();
}

if (address(_crossDomainMessenger) == address(0)) {
revert CannotSetCrossDomainMessengerToZero();
}

opWorldIDAddress = _opWorldIDAddress;
worldIDAddress = _worldIDIdentityManager;
crossDomainMessengerAddress = _crossDomainMessenger;
Expand Down Expand Up @@ -131,7 +158,9 @@ contract OpStateBridge is Ownable2Step {
/// @param _owner new owner (EOA or contract)
/// @param _isLocal true if new owner is on Optimism, false if it is a cross-domain owner
function transferOwnershipOp(address _owner, bool _isLocal) external onlyOwner {
require(_owner != address(0), "New owner can't be the zero address");
if (_owner == address(0)) {
revert CannotSetZeroOwner();
}

// The `encodeCall` function is strongly typed, so this checks that we are passing the
// correct data to the OP Stack chain bridge.
Expand Down Expand Up @@ -173,7 +202,9 @@ contract OpStateBridge is Ownable2Step {
/// @notice Sets the gas limit for the propagateRoot method
/// @param _opGasLimit The new gas limit for the propagateRoot method
function setGasLimitPropagateRoot(uint32 _opGasLimit) external onlyOwner {
require(_opGasLimit > 0, "Gas limit must be greater than 0");
if (_opGasLimit <= 0) {
revert GasLimitNotGreaterThanZero();
}

_gasLimitPropagateRoot = _opGasLimit;

Expand All @@ -183,7 +214,9 @@ contract OpStateBridge is Ownable2Step {
/// @notice Sets the gas limit for the SetRootHistoryExpiry method
/// @param _opGasLimit The new gas limit for the SetRootHistoryExpiry method
function setGasLimitSetRootHistoryExpiry(uint32 _opGasLimit) external onlyOwner {
require(_opGasLimit > 0, "Gas limit must be greater than 0");
if (_opGasLimit <= 0) {
revert GasLimitNotGreaterThanZero();
}

_gasLimitSetRootHistoryExpiry = _opGasLimit;

Expand All @@ -193,7 +226,9 @@ contract OpStateBridge is Ownable2Step {
/// @notice Sets the gas limit for the transferOwnershipOp method
/// @param _opGasLimit The new gas limit for the transferOwnershipOp method
function setGasLimitTransferOwnershipOp(uint32 _opGasLimit) external onlyOwner {
require(_opGasLimit > 0, "Gas limit must be greater than 0");
if (_opGasLimit <= 0) {
revert GasLimitNotGreaterThanZero();
}

_gasLimitTransferOwnership = _opGasLimit;

Expand Down
30 changes: 21 additions & 9 deletions src/PolygonStateBridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
/// EVENTS ///
///////////////////////////////////////////////////////////////////

/// @notice Emmitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
/// @notice Emitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
/// @param rootHistoryExpiry The new root history expiry
event SetRootHistoryExpiry(uint256 rootHistoryExpiry);

/// @notice Emmitted when the owner calls setFxChildTunnel for the first time
/// @notice Emitted when the owner calls setFxChildTunnel for the first time
event SetFxChildTunnel(address fxChildTunnel);

/// @notice Emmitted when a root is sent to PolygonWorldID
/// @notice Emitted when a root is sent to PolygonWorldID
/// @param root The latest WorldID Identity Manager root.
event RootPropagated(uint256 root);

Expand All @@ -43,6 +43,15 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
/// @notice Emitted when an attempt is made to renounce ownership.
error CannotRenounceOwnership();

/// @notice Emitted when an attempt is made to set the FxChildTunnel to the zero address.
error CannotSetFxRootToZero();

/// @notice Emitted when an attempt is made to set the CheckpointManager to the zero address.
error CannotSetCheckpointManagerToZero();

/// @notice Emitted when an attempt is made to set the WorldIDIdentityManager to the zero address.
error CannotSetWorldIDToZero();

///////////////////////////////////////////////////////////////////
/// CONSTRUCTOR ///
///////////////////////////////////////////////////////////////////
Expand All @@ -54,14 +63,17 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
constructor(address _checkpointManager, address _fxRoot, address _worldIDIdentityManager)
FxBaseRootTunnel(_checkpointManager, _fxRoot)
{
require(
_worldIDIdentityManager != address(0),
"WorldIDIdentityManager cannot be the zero address"
);
if (address(_checkpointManager) == address(0)) {
revert CannotSetCheckpointManagerToZero();
}

require(_fxRoot != address(0), "fxRoot cannot be the zero address");
if (address(_fxRoot) == address(0)) {
revert CannotSetFxRootToZero();
}

require(_checkpointManager != address(0), "checkpointManager cannot be the zero address");
if (address(_worldIDIdentityManager) == address(0)) {
revert CannotSetWorldIDToZero();
}

worldID = IWorldIDIdentityManager(_worldIDIdentityManager);
}
Expand Down
9 changes: 8 additions & 1 deletion src/PolygonWorldID.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ contract PolygonWorldID is WorldIDBridge, FxBaseChildTunnel, Ownable2Step {
/// @notice Emitted when an attempt is made to renounce ownership.
error CannotRenounceOwnership();

/// @notice Emitted when an attempt is made to set the FxChildTunnel to the zero address.
error CannotSetFxChildTunnelToZero();

///////////////////////////////////////////////////////////////////////////////
/// CONSTRUCTION ///
///////////////////////////////////////////////////////////////////////////////
Expand All @@ -56,7 +59,11 @@ contract PolygonWorldID is WorldIDBridge, FxBaseChildTunnel, Ownable2Step {
constructor(uint8 _treeDepth, address _fxChild)
WorldIDBridge(_treeDepth)
FxBaseChildTunnel(_fxChild)
{}
{
if (address(_fxChild) == address(0)) {
revert CannotSetFxChildTunnelToZero();
dcbuild3r marked this conversation as resolved.
Show resolved Hide resolved
}
}

///////////////////////////////////////////////////////////////////////////////
/// ROOT MIRRORING ///
Expand Down
1 change: 0 additions & 1 deletion src/abstract/WorldIDBridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ abstract contract WorldIDBridge is IWorldID {
/// @param newRoot The value of the new root.
///
/// @custom:reverts CannotOverwriteRoot If the root already exists in the root history.
/// @custom:reverts string If the caller is not the owner.
function _receiveRoot(uint256 newRoot) internal {
uint256 existingTimestamp = rootHistory[newRoot];

Expand Down
64 changes: 52 additions & 12 deletions src/test/OpStateBridge.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,17 @@ contract OpStateBridgeTest is PRBTest, StdCheats {
/// EVENTS ///
///////////////////////////////////////////////////////////////////

/// @notice Emmitted when the ownership transfer of OpStateBridge is started (OZ Ownable2Step)
/// @notice Emitted when the ownership transfer of OpStateBridge is started (OZ Ownable2Step)
event OwnershipTransferStarted(address indexed previousOwner, address indexed newOwner);

/// @notice Emmitted when the ownership transfer of OpStateBridge is accepted (OZ Ownable2Step)
/// @notice Emitted when the ownership transfer of OpStateBridge is accepted (OZ Ownable2Step)
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);

// @notice Emmitted when the the StateBridge sends a root to the OPWorldID contract
// @notice Emitted when the the StateBridge sends a root to the OPWorldID contract
/// @param root The root sent to the OPWorldID contract on the OP Stack chain
event RootPropagated(uint256 root);

/// @notice Emmitted when the the StateBridge gives ownership of the OPWorldID contract
/// @notice Emitted when the the StateBridge gives ownership of the OPWorldID contract
/// to the WorldID Identity Manager contract away
/// @param previousOwner The previous owner of the OPWorldID contract
/// @param newOwner The new owner of the OPWorldID contract
Expand All @@ -61,19 +61,19 @@ contract OpStateBridgeTest is PRBTest, StdCheats {
address indexed previousOwner, address indexed newOwner, bool isLocal
);

/// @notice Emmitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
/// @notice Emitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
/// @param rootHistoryExpiry The new root history expiry
event SetRootHistoryExpiry(uint256 rootHistoryExpiry);

/// @notice Emmitted when the the StateBridge sets the gas limit for sendRootOp
/// @notice Emitted when the the StateBridge sets the gas limit for sendRootOp
/// @param _opGasLimit The new opGasLimit for sendRootOp
event SetGasLimitPropagateRoot(uint32 _opGasLimit);

/// @notice Emmitted when the the StateBridge sets the gas limit for SetRootHistoryExpiryt
/// @notice Emitted when the the StateBridge sets the gas limit for SetRootHistoryExpiryt
/// @param _opGasLimit The new opGasLimit for SetRootHistoryExpirytimism
event SetGasLimitSetRootHistoryExpiry(uint32 _opGasLimit);

/// @notice Emmitted when the the StateBridge sets the gas limit for transferOwnershipOp
/// @notice Emitted when the the StateBridge sets the gas limit for transferOwnershipOp
/// @param _opGasLimit The new opGasLimit for transferOwnershipOptimism
event SetGasLimitTransferOwnershipOp(uint32 _opGasLimit);

Expand All @@ -84,6 +84,22 @@ contract OpStateBridgeTest is PRBTest, StdCheats {
/// @notice Emitted when an attempt is made to renounce ownership.
error CannotRenounceOwnership();

/// @notice Emitted when an attempt is made to set the gas limit to zero or less
error GasLimitNotGreaterThanZero();
dcbuild3r marked this conversation as resolved.
Show resolved Hide resolved

/// @notice Emitted when an attempt is made to set the owner to the zero address
error CannotSetZeroOwner();

/// @notice Emitted when an attempt is made to set the World ID Identity Manager to the zero address
error CannotSetWorldIDToZero();

/// @notice Emitted when an attempt is made to set the Op World ID Identity Manager to the zero address
error CannotSetOpWorldIDToZero();

/// @notice Emitted when an attempt is made to set the Cross Domain Messenger to the zero address
error CannotSetCrossDomainMessengerToZero();
dcbuild3r marked this conversation as resolved.
Show resolved Hide resolved


function setUp() public {
/// @notice Create a fork of the Ethereum mainnet
mainnetFork = vm.createFork(MAINNET_RPC_URL);
Expand Down Expand Up @@ -227,6 +243,30 @@ contract OpStateBridgeTest is PRBTest, StdCheats {
/// REVERTS ///
///////////////////////////////////////////////////////////////////

/// @notice Tests that the StateBridge constructor params can't be set to the zero address
function test_cannotInitializeConstructorWithZeroAddresses_reverts() public {
vm.expectRevert(CannotSetWorldIDToZero.selector);
opStateBridge = new OpStateBridge(
address(0),
opWorldIDAddress,
opCrossDomainMessengerAddress
);

vm.expectRevert(CannotSetOpWorldIDToZero.selector);
opStateBridge = new OpStateBridge(
mockWorldIDAddress,
address(0),
opCrossDomainMessengerAddress
);

vm.expectRevert(CannotSetCrossDomainMessengerToZero.selector);
opStateBridge = new OpStateBridge(
mockWorldIDAddress,
opWorldIDAddress,
address(0)
);
}

/// @notice tests that the StateBridge contract's ownership can't be changed by a non-owner
/// @param newOwner The new owner of the StateBridge contract (foundry fuzz)
/// @param nonOwner An address that is not the owner of the StateBridge contract
Expand All @@ -241,7 +281,7 @@ contract OpStateBridgeTest is PRBTest, StdCheats {

/// @notice tests that the StateBridge contract's ownership can't be set to be the zero address
function test_owner_transferOwnershipOp_toZeroAddress_reverts() public {
vm.expectRevert("New owner can't be the zero address");
vm.expectRevert(CannotSetZeroOwner.selector);

vm.prank(owner);
opStateBridge.transferOwnershipOp(address(0), true);
Expand Down Expand Up @@ -304,17 +344,17 @@ contract OpStateBridgeTest is PRBTest, StdCheats {

/// @notice Tests that the StateBridge contract can't set the opGasLimit for sendRootOptimism to 0
function test_setGasLimitToZero_reverts() public {
vm.expectRevert("Gas limit must be greater than 0");
vm.expectRevert(GasLimitNotGreaterThanZero.selector);

vm.prank(owner);
opStateBridge.setGasLimitPropagateRoot(0);

vm.expectRevert("Gas limit must be greater than 0");
vm.expectRevert(GasLimitNotGreaterThanZero.selector);

vm.prank(owner);
opStateBridge.setGasLimitSetRootHistoryExpiry(0);

vm.expectRevert("Gas limit must be greater than 0");
vm.expectRevert(GasLimitNotGreaterThanZero.selector);

vm.prank(owner);
opStateBridge.setGasLimitTransferOwnershipOp(0);
Expand Down
21 changes: 15 additions & 6 deletions src/test/PolygonStateBridge.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,26 @@ contract PolygonStateBridgeTest is PRBTest, StdCheats {
/// @param newOwner The new owner of the StateBridge contract
event OwnershipTransferStarted(address indexed previousOwner, address indexed newOwner);

/// @notice Emmitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
/// @notice Emitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
/// @param rootHistoryExpiry The new root history expiry
event SetRootHistoryExpiry(uint256 rootHistoryExpiry);

// @notice Emmitted when the owner calls setFxChildTunnel for the first time
// @notice Emitted when the owner calls setFxChildTunnel for the first time
event SetFxChildTunnel(address fxChildTunnel);

/// @notice Emmitted when a root is sent to PolygonWorldID
/// @notice Emitted when a root is sent to PolygonWorldID
/// @param root The latest WorldID Identity Manager root.
event RootPropagated(uint256 root);

/// @notice Emitted when an attempt is made to set the FxChildTunnel to the zero address.
error CannotSetFxRootToZero();

/// @notice Emitted when an attempt is made to set the CheckpointManager to the zero address.
error CannotSetCheckpointManagerToZero();

/// @notice Emitted when an attempt is made to set the WorldIDIdentityManager to the zero address.
error CannotSetWorldIDToZero();

function setUp() public {
/// @notice Create a fork of the Ethereum mainnet
mainnetFork = vm.createFork(MAINNET_RPC_URL);
Expand Down Expand Up @@ -160,21 +169,21 @@ contract PolygonStateBridgeTest is PRBTest, StdCheats {

/// @notice tests that the StateBridge contract can't be constructed with a zero address for params
function test_constructorParamsCannotBeZeroAddresses_reverts() public {
vm.expectRevert("WorldIDIdentityManager cannot be the zero address");
vm.expectRevert(CannotSetWorldIDToZero.selector);
polygonStateBridge = new PolygonStateBridge(
checkpointManager,
fxRoot,
address(0)
);

vm.expectRevert("fxRoot cannot be the zero address");
vm.expectRevert(CannotSetFxRootToZero.selector);
polygonStateBridge = new PolygonStateBridge(
checkpointManager,
address(0),
mockWorldIDAddress
);

vm.expectRevert("checkpointManager cannot be the zero address");
vm.expectRevert(CannotSetCheckpointManagerToZero.selector);
polygonStateBridge = new PolygonStateBridge(
address(0),
fxRoot,
Expand Down
Loading
Loading