From 0de67bdf11a629eb3825ab305beec2c0f7d8cb38 Mon Sep 17 00:00:00 2001 From: josojo Date: Mon, 1 Jan 2024 15:32:20 +0100 Subject: [PATCH] First round of revert string removals (#139) * First round of revert string removements * add revert errors in forkable bridge --- contracts/ForkableBridge.sol | 8 +++-- contracts/interfaces/IForkableBridge.sol | 5 +++ contracts/interfaces/IForkableStructure.sol | 12 +++++++ contracts/mixin/ForkableStructure.sol | 25 ++++++++------ test/ForkableBridge.t.sol | 36 +++++++++++++-------- test/ForkableGlobalExitRoot.t.sol | 3 +- test/ForkableZkEVM.t.sol | 11 ++++--- test/ForkingManager.t.sol | 3 +- test/L1GlobalChainInfoPublisher.t.sol | 5 +-- 9 files changed, 73 insertions(+), 35 deletions(-) diff --git a/contracts/ForkableBridge.sol b/contracts/ForkableBridge.sol index 4d11a52a..2f3968f0 100644 --- a/contracts/ForkableBridge.sol +++ b/contracts/ForkableBridge.sol @@ -56,8 +56,12 @@ contract ForkableBridge is uint256 amount, address to ) external onlyAfterForking { - require(_hardAssetManager == msg.sender, "Not authorized"); - require(to == children[0] || to == children[1], "Invalid to"); + if (_hardAssetManager != msg.sender) { + revert NotAuthorized(); + } + if (to != children[0] && to != children[1]) { + revert InvalidDestinationForHardAsset(); + } IERC20(token).transfer(to, amount); } diff --git a/contracts/interfaces/IForkableBridge.sol b/contracts/interfaces/IForkableBridge.sol index 68acd364..5ba2aefb 100644 --- a/contracts/interfaces/IForkableBridge.sol +++ b/contracts/interfaces/IForkableBridge.sol @@ -7,6 +7,11 @@ import {IBasePolygonZkEVMGlobalExitRoot} from "@RealityETH/zkevm-contracts/contr import {IForkableStructure} from "./IForkableStructure.sol"; interface IForkableBridge is IForkableStructure, IPolygonZkEVMBridge { + /// @dev Error thrown when activity is started by a non-authorized actor + error NotAuthorized(); + /// @dev Error thrown when trying to send bridged tokens to a child contract + error InvalidDestinationForHardAsset(); + /** * @dev Function to initialize the contract * @param _forkmanager: address of the forkmanager contract diff --git a/contracts/interfaces/IForkableStructure.sol b/contracts/interfaces/IForkableStructure.sol index a8543ee6..7a7de94f 100644 --- a/contracts/interfaces/IForkableStructure.sol +++ b/contracts/interfaces/IForkableStructure.sol @@ -3,9 +3,21 @@ pragma solidity ^0.8.20; interface IForkableStructure { + /// @dev Error thrown when trying to call a function that can only be called after forking + error NoChangesAfterForking(); + /// @dev Error thrown when trying to call a function that can only be called before forking + error OnlyAfterForking(); + /// @dev Error thrown when trying to call a function that can only be called by the parent contract + error OnlyParentIsAllowed(); + /// @dev Error thrown when trying to call a function that can only be called by the forkmanager + error OnlyForkManagerIsAllowed(); + + /// @dev Returns the address of the forkmanager function forkmanager() external view returns (address); + /// @dev Returns the address of the parent contract function parentContract() external view returns (address); + /// @dev Returns the addresses of the two children function getChildren() external view returns (address, address); } diff --git a/contracts/mixin/ForkableStructure.sol b/contracts/mixin/ForkableStructure.sol index 3c7e7a1d..6b79a283 100644 --- a/contracts/mixin/ForkableStructure.sol +++ b/contracts/mixin/ForkableStructure.sol @@ -18,28 +18,33 @@ contract ForkableStructure is IForkableStructure, Initializable { mapping(uint256 => address) public children; modifier onlyBeforeForking() { - require(children[0] == address(0x0), "No changes after forking"); - _; - } - - modifier onlyBeforeCreatingChild2() { - require(children[2] == address(0x0), "No changes after forking"); + if (children[0] != address(0x0)) { + revert NoChangesAfterForking(); + } _; } modifier onlyAfterForking() { - require(children[0] != address(0x0), "onlyAfterForking"); - require(children[1] != address(0x0), "onlyAfterForking"); + if (children[0] == address(0x0)) { + revert OnlyAfterForking(); + } + if (children[1] == address(0x0)) { + revert OnlyAfterForking(); + } _; } modifier onlyParent() { - require(msg.sender == parentContract, "Not parent"); + if (msg.sender != parentContract) { + revert OnlyParentIsAllowed(); + } _; } modifier onlyForkManger() { - require(msg.sender == forkmanager, "Not forkManager"); + if (msg.sender != forkmanager) { + revert OnlyForkManagerIsAllowed(); + } _; } diff --git a/test/ForkableBridge.t.sol b/test/ForkableBridge.t.sol index 8d423d0c..72536289 100644 --- a/test/ForkableBridge.t.sol +++ b/test/ForkableBridge.t.sol @@ -4,6 +4,8 @@ import {Test} from "forge-std/Test.sol"; import {ForkableBridge} from "../contracts/ForkableBridge.sol"; import {ForkableBridgeWrapper} from "./testcontract/ForkableBridgeWrapper.sol"; import {ForkonomicToken} from "../contracts/ForkonomicToken.sol"; +import {IForkableBridge} from "../contracts/interfaces/IForkableBridge.sol"; +import {IForkableStructure} from "../contracts/interfaces/IForkableStructure.sol"; import {IForkonomicToken} from "../contracts/interfaces/IForkonomicToken.sol"; import {ForkableGlobalExitRoot} from "../contracts/ForkableGlobalExitRoot.sol"; import {IERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/interfaces/IERC20Upgradeable.sol"; @@ -69,7 +71,7 @@ contract ForkableBridgeTest is Test { } function testCreateChildren() public { - vm.expectRevert(bytes("Not forkManager")); + vm.expectRevert(IForkableStructure.OnlyForkManagerIsAllowed.selector); forkableBridge.createChildren(); ForkableGlobalExitRoot exitRoot = new ForkableGlobalExitRoot(); vm.mockCall( @@ -96,7 +98,7 @@ contract ForkableBridgeTest is Test { abi.encodePacked(originNetwork, token) ); - vm.expectRevert(bytes("Not parent")); + vm.expectRevert(IForkableStructure.OnlyParentIsAllowed.selector); forkableBridge.mintForkableToken( address(token), originNetwork, @@ -173,7 +175,7 @@ contract ForkableBridgeTest is Test { amount ); - vm.expectRevert(bytes("Not parent")); + vm.expectRevert(IForkableStructure.OnlyParentIsAllowed.selector); forkableBridge.burnForkableTokens( destinationAddress, originTokenAddress, @@ -213,7 +215,8 @@ contract ForkableBridgeTest is Test { ); // Testing revert if children are not yet created - vm.expectRevert(bytes("onlyAfterForking")); + vm.expectRevert(IForkableStructure.OnlyAfterForking.selector); + forkableBridge.splitTokenIntoChildToken(address(token), amount, true); vm.prank(forkableBridge.forkmanager()); @@ -345,7 +348,8 @@ contract ForkableBridgeTest is Test { forkableBridge.splitTokenIntoChildToken(forkableToken, amount, true); // Only parent can merge - vm.expectRevert(bytes("onlyAfterForking")); + vm.expectRevert(IForkableStructure.OnlyAfterForking.selector); + ForkableBridge(child1).mergeChildTokens(forkableToken, amount + 1); // Merge the token @@ -387,7 +391,7 @@ contract ForkableBridgeTest is Test { vm.prank(forkableBridge.forkmanager()); forkableBridge.createChildren(); - vm.expectRevert(bytes("No changes after forking")); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); forkableBridge.bridgeAsset( 12, address(0x3), @@ -397,11 +401,11 @@ contract ForkableBridgeTest is Test { bytes("0x3453") ); - vm.expectRevert(bytes("No changes after forking")); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); forkableBridge.bridgeMessage(2, address(0x45), false, bytes("0x3453")); bytes32[32] memory smtProof; - vm.expectRevert(bytes("No changes after forking")); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); forkableBridge.claimMessage( smtProof, 2, @@ -415,7 +419,7 @@ contract ForkableBridgeTest is Test { bytes("0x") ); - vm.expectRevert(bytes("No changes after forking")); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); forkableBridge.claimAsset( smtProof, 32, @@ -447,7 +451,7 @@ contract ForkableBridgeTest is Test { false, "" ); - vm.expectRevert("onlyAfterForking"); + vm.expectRevert(IForkableStructure.OnlyAfterForking.selector); vm.prank(hardAssetManger); forkableBridge.transferHardAssetsToChild( address(erc20Token), @@ -493,14 +497,16 @@ contract ForkableBridgeTest is Test { depositTreeHashes ); - vm.expectRevert("Not authorized"); + vm.expectRevert(IForkableBridge.NotAuthorized.selector); forkableBridge.transferHardAssetsToChild( address(erc20Token), amount, to ); - vm.expectRevert("Invalid to"); + vm.expectRevert( + IForkableBridge.InvalidDestinationForHardAsset.selector + ); vm.prank(hardAssetManger); forkableBridge.transferHardAssetsToChild( address(erc20Token), @@ -561,10 +567,12 @@ contract ForkableBridgeTest is Test { erc20GasToken.mint(address(forkableBridge2), mintAmount); // Now, call the function as the forkmanager - vm.expectRevert(bytes("onlyAfterForking")); + vm.expectRevert(IForkableStructure.OnlyAfterForking.selector); + vm.prank(forkmanager); forkableBridge2.sendForkonomicTokensToChild(10, true, false); - vm.expectRevert(bytes("onlyAfterForking")); + vm.expectRevert(IForkableStructure.OnlyAfterForking.selector); + vm.prank(forkmanager); forkableBridge2.sendForkonomicTokensToChild(10, true, false); diff --git a/test/ForkableGlobalExitRoot.t.sol b/test/ForkableGlobalExitRoot.t.sol index c26a7226..6d0c1aa0 100644 --- a/test/ForkableGlobalExitRoot.t.sol +++ b/test/ForkableGlobalExitRoot.t.sol @@ -1,6 +1,7 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; +import {IForkableStructure} from "../contracts/interfaces/IForkableStructure.sol"; import {ForkableGlobalExitRoot} from "../contracts/ForkableGlobalExitRoot.sol"; import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; import {Util} from "./utils/Util.sol"; @@ -69,7 +70,7 @@ contract ForkableGlobalExitRootTest is Test { } function testCreateChildrenOnlyByForkManager() public { - vm.expectRevert("Not forkManager"); + vm.expectRevert(IForkableStructure.OnlyForkManagerIsAllowed.selector); forkableGlobalExitRoot.createChildren(); vm.prank(forkableGlobalExitRoot.forkmanager()); forkableGlobalExitRoot.createChildren(); diff --git a/test/ForkableZkEVM.t.sol b/test/ForkableZkEVM.t.sol index 9ee7dd28..aa2c08f1 100644 --- a/test/ForkableZkEVM.t.sol +++ b/test/ForkableZkEVM.t.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; import {PolygonZkEVM} from "@RealityETH/zkevm-contracts/contracts/inheritedMainContracts/PolygonZkEVM.sol"; import {ForkableZkEVM} from "../contracts/ForkableZkEVM.sol"; +import {IForkableStructure} from "../contracts/interfaces/IForkableStructure.sol"; import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; import {IPolygonZkEVMGlobalExitRoot} from "@RealityETH/zkevm-contracts/contracts/interfaces/IPolygonZkEVMGlobalExitRoot.sol"; import {IVerifierRollup} from "@RealityETH/zkevm-contracts/contracts/interfaces/IVerifierRollup.sol"; @@ -93,7 +94,7 @@ contract ForkableZkEVMTest is Test { } function testCreateChildren() public { - vm.expectRevert("Not forkManager"); + vm.expectRevert(IForkableStructure.OnlyForkManagerIsAllowed.selector); forkableZkEVM.createChildren(); vm.prank(forkableZkEVM.forkmanager()); @@ -130,7 +131,7 @@ contract ForkableZkEVMTest is Test { vm.prank(forkableZkEVM.forkmanager()); forkableZkEVM.createChildren(); - vm.expectRevert("No changes after forking"); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); forkableZkEVM.verifyBatches( pendingStateNum, initNumBatch, @@ -145,7 +146,7 @@ contract ForkableZkEVMTest is Test { vm.prank(forkableZkEVM.forkmanager()); forkableZkEVM.createChildren(); - vm.expectRevert("No changes after forking"); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); forkableZkEVM.consolidatePendingState(10); bytes32[24] memory proof; @@ -153,7 +154,7 @@ contract ForkableZkEVMTest is Test { proof[i] = bytes32("0x"); // Whatever initialization value you want } - vm.expectRevert("No changes after forking"); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); forkableZkEVM.overridePendingState( 10, 10, @@ -164,7 +165,7 @@ contract ForkableZkEVMTest is Test { proof ); - vm.expectRevert("No changes after forking"); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); PolygonZkEVM.BatchData[] memory batches = new PolygonZkEVM.BatchData[]( 1 ); diff --git a/test/ForkingManager.t.sol b/test/ForkingManager.t.sol index cdfeb8af..c6334004 100644 --- a/test/ForkingManager.t.sol +++ b/test/ForkingManager.t.sol @@ -9,6 +9,7 @@ import {ForkableBridge} from "../contracts/ForkableBridge.sol"; import {ForkableZkEVM} from "../contracts/ForkableZkEVM.sol"; import {ForkonomicToken} from "../contracts/ForkonomicToken.sol"; import {ForkableGlobalExitRoot} from "../contracts/ForkableGlobalExitRoot.sol"; +import {IForkableStructure} from "../contracts/interfaces/IForkableStructure.sol"; import {IPolygonZkEVMGlobalExitRoot} from "@RealityETH/zkevm-contracts/contracts/interfaces/IPolygonZkEVMGlobalExitRoot.sol"; import {IForkingManager} from "../contracts/interfaces/IForkingManager.sol"; import {IVerifierRollup} from "@RealityETH/zkevm-contracts/contracts/interfaces/IVerifierRollup.sol"; @@ -641,7 +642,7 @@ contract ForkingManagerTest is Test { forkmanager.initiateFork(disputeData); skip(forkmanager.forkPreparationTime() + 1); forkmanager.executeFork(); - vm.expectRevert("No changes after forking"); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); forkmanager.executeFork(); } diff --git a/test/L1GlobalChainInfoPublisher.t.sol b/test/L1GlobalChainInfoPublisher.t.sol index a4fd4467..fd9437f5 100644 --- a/test/L1GlobalChainInfoPublisher.t.sol +++ b/test/L1GlobalChainInfoPublisher.t.sol @@ -19,6 +19,7 @@ import {ForkableRealityETH_ERC20} from "../contracts/ForkableRealityETH_ERC20.so import {RealityETH_v3_0} from "../contracts/lib/reality-eth/RealityETH-3.0.sol"; import {AdjudicationFramework} from "../contracts/AdjudicationFramework.sol"; +import {IForkableStructure} from "../contracts/interfaces/IForkableStructure.sol"; import {L2ForkArbitrator} from "../contracts/L2ForkArbitrator.sol"; import {L1GlobalChainInfoPublisher} from "../contracts/L1GlobalChainInfoPublisher.sol"; import {L1GlobalForkRequester} from "../contracts/L1GlobalForkRequester.sol"; @@ -290,7 +291,7 @@ contract L1GlobalChainInfoPublisherTest is Test { forkmanager.executeFork(); // The current bridge should no longer work - vm.expectRevert("No changes after forking"); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); l1GlobalChainInfoPublisher.updateL2ChainInfo( address(bridge), address(l2ChainInfo), @@ -352,7 +353,7 @@ contract L1GlobalChainInfoPublisherTest is Test { skip(forkmanager.forkPreparationTime() + 1); forkmanager2.executeFork(); - vm.expectRevert("No changes after forking"); + vm.expectRevert(IForkableStructure.NoChangesAfterForking.selector); l1GlobalChainInfoPublisher.updateL2ChainInfo( bridge2, address(l2ChainInfo),