From a82b25bae571a3882eb045026f6ee9d49ddf5317 Mon Sep 17 00:00:00 2001 From: josojo Date: Fri, 5 Jan 2024 08:44:49 +0100 Subject: [PATCH] Refactors of the smart contracts - using error instead of reverts (#167) * refactors of the smart contracts - using error instead of reverts and minor modifications * linting --- contracts/ChainIdManager.sol | 6 +++--- contracts/ForkableBridge.sol | 4 +++- contracts/ForkingManager.sol | 17 +++++++++------- contracts/ForkonomicToken.sol | 18 +++++++---------- contracts/interfaces/IForkableBridge.sol | 2 ++ contracts/interfaces/IForkingManager.sol | 5 +++++ contracts/interfaces/IForkonomicToken.sol | 5 +++++ contracts/interfaces/IOperations.sol | 16 --------------- contracts/lib/BridgeAssetOperations.sol | 20 +++++++++++++------ contracts/lib/reality-eth/Arbitrator.sol | 2 +- .../{mixin => lib/reality-eth}/Owned.sol | 0 contracts/mixin/InitializeChain.sol | 3 +++ test/ChainIdManager.t.sol | 2 +- test/ForkableBridge.t.sol | 2 +- test/ForkingManager.t.sol | 6 +++--- test/ForkonomicToken.t.sol | 2 +- 16 files changed, 59 insertions(+), 51 deletions(-) delete mode 100644 contracts/interfaces/IOperations.sol rename contracts/{mixin => lib/reality-eth}/Owned.sol (100%) diff --git a/contracts/ChainIdManager.sol b/contracts/ChainIdManager.sol index 698f3c27..1e448eca 100644 --- a/contracts/ChainIdManager.sol +++ b/contracts/ChainIdManager.sol @@ -1,9 +1,9 @@ // SPDX-License-Identifier: GPL-3.0-only pragma solidity ^0.8.20; -import {Owned} from "./mixin/Owned.sol"; +import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; -contract ChainIdManager is Owned { +contract ChainIdManager is Ownable { // Counter for the number of Chain IDs uint64 public chainIdCounter = 0; // contains a list of denied Chain IDs that should not be used as chainIds @@ -12,7 +12,7 @@ contract ChainIdManager is Owned { // Fee to use up a Chain ID uint256 public immutable gasBurnAmount = 1000000; - constructor(uint64 _chainIdCounter) Owned() { + constructor(uint64 _chainIdCounter) Ownable() { chainIdCounter = _chainIdCounter; } diff --git a/contracts/ForkableBridge.sol b/contracts/ForkableBridge.sol index 38c65292..d3701244 100644 --- a/contracts/ForkableBridge.sol +++ b/contracts/ForkableBridge.sol @@ -124,7 +124,9 @@ contract ForkableBridge is bytes calldata metadata, address destinationAddress ) external onlyParent { - require(originNetwork != networkID, "wrong Token"); + if (originNetwork == networkID) { + revert InvalidOriginNetwork(); + } _issueBridgedTokens( originNetwork, token, diff --git a/contracts/ForkingManager.sol b/contracts/ForkingManager.sol index 8b1d9d92..1cedce72 100644 --- a/contracts/ForkingManager.sol +++ b/contracts/ForkingManager.sol @@ -85,7 +85,9 @@ contract ForkingManager is IForkingManager, ForkableStructure { function initiateFork( DisputeData memory _disputeData ) external onlyBeforeForking { - require(executionTimeForProposal == 0, "ForkingManager: fork pending"); + if (executionTimeForProposal != 0) { + revert ForkingAlreadyInitiated(); + } // Charge the forking fee IERC20(forkonomicToken).safeTransferFrom( msg.sender, @@ -106,12 +108,13 @@ contract ForkingManager is IForkingManager, ForkableStructure { * @dev function that executes a fork proposal */ function executeFork() external onlyBeforeForking { - require( - executionTimeForProposal != 0 && - // solhint-disable-next-line not-rely-on-time - executionTimeForProposal <= block.timestamp, - "ForkingManager: fork not ready" - ); + if ( + executionTimeForProposal == 0 || + // solhint-disable-next-line not-rely-on-time + executionTimeForProposal > block.timestamp + ) { + revert NotYetReadyToFork(); + } // Create the children of each contract NewInstances memory newInstances; diff --git a/contracts/ForkonomicToken.sol b/contracts/ForkonomicToken.sol index a381a56b..65242169 100644 --- a/contracts/ForkonomicToken.sol +++ b/contracts/ForkonomicToken.sol @@ -38,10 +38,9 @@ contract ForkonomicToken is /// @inheritdoc IForkonomicToken function mint(address to, uint256 amount) external { - require( - hasRole(MINTER_ROLE, msg.sender) || msg.sender == parentContract, - "Caller is not a minter" - ); + if (!hasRole(MINTER_ROLE, msg.sender) && msg.sender != parentContract) { + revert NotMinterRole(); + } _mint(to, amount); } @@ -59,12 +58,10 @@ contract ForkonomicToken is bool firstChild, bool useChildTokenAllowance ) public onlyAfterForking { - require(children[0] != address(0), "Children not created yet"); if (useChildTokenAllowance) { - require( - childTokenAllowances[msg.sender][firstChild] >= amount, - "Not enough allowance" - ); + if (childTokenAllowances[msg.sender][firstChild] < amount) { + revert NotSufficientAllowance(); + } childTokenAllowances[msg.sender][firstChild] -= amount; } else { _burn(msg.sender, amount); @@ -79,8 +76,7 @@ contract ForkonomicToken is /// @dev Allows anyone to prepare the splitting of tokens /// by burning them /// @param amount The amount of tokens to burn - function prepareSplittingTokens(uint256 amount) public { - require(children[0] != address(0), "Children not created yet"); + function prepareSplittingTokens(uint256 amount) public onlyAfterForking { _burn(msg.sender, amount); childTokenAllowances[msg.sender][false] += amount; childTokenAllowances[msg.sender][true] += amount; diff --git a/contracts/interfaces/IForkableBridge.sol b/contracts/interfaces/IForkableBridge.sol index 30c7125d..0b00c517 100644 --- a/contracts/interfaces/IForkableBridge.sol +++ b/contracts/interfaces/IForkableBridge.sol @@ -13,6 +13,8 @@ interface IForkableBridge is IForkableStructure, IPolygonZkEVMBridge { error InvalidDestinationForHardAsset(); /// @dev Error thrown when hardasset manager tries to send gas token to a child contract error GasTokenIsNotHardAsset(); + /// @dev Error thrown when trying to send bridged tokens to a child contract + error InvalidOriginNetwork(); /** * @dev Function to initialize the contract diff --git a/contracts/interfaces/IForkingManager.sol b/contracts/interfaces/IForkingManager.sol index 8a9086a5..2332f688 100644 --- a/contracts/interfaces/IForkingManager.sol +++ b/contracts/interfaces/IForkingManager.sol @@ -4,6 +4,11 @@ pragma solidity ^0.8.20; import {IForkableStructure} from "./IForkableStructure.sol"; interface IForkingManager is IForkableStructure { + /// @dev Error thrown when the forking manager is not ready to fork + error NotYetReadyToFork(); + /// @dev Error thrown when the forking manager is already initiated + error ForkingAlreadyInitiated(); + // Dispute contract and call to identify the dispute // that will be used to initiate/justify the fork struct DisputeData { diff --git a/contracts/interfaces/IForkonomicToken.sol b/contracts/interfaces/IForkonomicToken.sol index c608b218..a973e6ef 100644 --- a/contracts/interfaces/IForkonomicToken.sol +++ b/contracts/interfaces/IForkonomicToken.sol @@ -5,6 +5,11 @@ import {IForkableStructure} from "./IForkableStructure.sol"; import {IERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; interface IForkonomicToken is IForkableStructure, IERC20Upgradeable { + /// @dev Error thrown when activity is started by a non-authorized minter + error NotMinterRole(); + /// @dev Error thrown when not sufficient balance has been burn to mint new tokens at the child contracts + error NotSufficientAllowance(); + /** * @notice Allows the forkmanager to initialize the contract * @param _forkmanager The address of the forkmanager diff --git a/contracts/interfaces/IOperations.sol b/contracts/interfaces/IOperations.sol deleted file mode 100644 index 56df5b0a..00000000 --- a/contracts/interfaces/IOperations.sol +++ /dev/null @@ -1,16 +0,0 @@ -// SPDX-License-Identifier: MIT OR Apache-2.0 - -pragma solidity ^0.8.10; - -library Operations { - enum OpTree { - Full, - Rollup - } - - enum QueueType { - Deque, - HeapBuffer, - Heap - } -} diff --git a/contracts/lib/BridgeAssetOperations.sol b/contracts/lib/BridgeAssetOperations.sol index 5403ff0a..8c7e1d2f 100644 --- a/contracts/lib/BridgeAssetOperations.sol +++ b/contracts/lib/BridgeAssetOperations.sol @@ -8,6 +8,11 @@ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; library BridgeAssetOperations { + // @dev Error thrown when non-forkable token is intended to be used, but it is not forkable + error TokenNotForkable(); + // @dev Error thrown when token is not issued before + error TokenNotIssuedBefore(); + /** * @notice Function to merge tokens from children-bridge contracts * @param token Address of the token @@ -23,11 +28,12 @@ library BridgeAssetOperations { address child0, address child1 ) public { - require(tokenInfo.originNetwork != 0, "Token not forkable"); - require( - tokenInfo.originTokenAddress != address(0), - "Token not issued before" - ); + if (tokenInfo.originNetwork == 0) { + revert TokenNotForkable(); + } + if (tokenInfo.originTokenAddress == address(0)) { + revert TokenNotIssuedBefore(); + } ForkableBridge(child0).burnForkableTokens( msg.sender, tokenInfo.originTokenAddress, @@ -57,7 +63,9 @@ library BridgeAssetOperations { PolygonZkEVMBridge.TokenInformation memory tokenInfo, address child ) public { - require(tokenInfo.originNetwork != 0, "Token not forkable"); + if (tokenInfo.originNetwork == 0) { + revert TokenNotForkable(); + } bytes memory metadata = abi.encode( IERC20Metadata(token).name(), IERC20Metadata(token).symbol(), diff --git a/contracts/lib/reality-eth/Arbitrator.sol b/contracts/lib/reality-eth/Arbitrator.sol index 48de8249..f2c74e02 100644 --- a/contracts/lib/reality-eth/Arbitrator.sol +++ b/contracts/lib/reality-eth/Arbitrator.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.20; import "./../../interfaces/IArbitrator.sol"; import "./../../interfaces/IRealityETH.sol"; import "./../../interfaces/IERC20.sol"; -import "./../../mixin/Owned.sol"; +import "./Owned.sol"; contract Arbitrator is Owned, IArbitrator { IRealityETH public realitio; diff --git a/contracts/mixin/Owned.sol b/contracts/lib/reality-eth/Owned.sol similarity index 100% rename from contracts/mixin/Owned.sol rename to contracts/lib/reality-eth/Owned.sol diff --git a/contracts/mixin/InitializeChain.sol b/contracts/mixin/InitializeChain.sol index 58dcf42a..6625a92f 100644 --- a/contracts/mixin/InitializeChain.sol +++ b/contracts/mixin/InitializeChain.sol @@ -1,6 +1,9 @@ // SPDX-License-Identifier: GPL-3.0-only pragma solidity ^0.8.20; +// Currently, this contract is not used. Ed came up with its own isChainUpToDate() modifier. +// Maybe we have to delete it. + /** This contract can be inherited form any other contrac that needs to be aware of Forks. This contract uses the fact that after a fork the chainId changes and thereby detects forks*/ diff --git a/test/ChainIdManager.t.sol b/test/ChainIdManager.t.sol index f629770c..852e9cfa 100644 --- a/test/ChainIdManager.t.sol +++ b/test/ChainIdManager.t.sol @@ -29,7 +29,7 @@ contract ChainIdManagerTest is Test { // Attempt to add a ChainId by a non-owner, expect a revert vm.prank(nonOwner); - vm.expectRevert(bytes("Caller is not the owner")); // Expect a revert with a specific revert message + vm.expectRevert(bytes("Ownable: caller is not the owner")); // Expect a revert with a specific revert message chainIdManager.denyListChainId(2); } diff --git a/test/ForkableBridge.t.sol b/test/ForkableBridge.t.sol index 1f64c1f7..8851e176 100644 --- a/test/ForkableBridge.t.sol +++ b/test/ForkableBridge.t.sol @@ -108,7 +108,7 @@ contract ForkableBridgeTest is Test { ); vm.prank(forkableBridge.parentContract()); - vm.expectRevert(bytes("wrong Token")); + vm.expectRevert(IForkableBridge.InvalidOriginNetwork.selector); forkableBridge.mintForkableToken( address(token), networkID, // <-- this line is changed diff --git a/test/ForkingManager.t.sol b/test/ForkingManager.t.sol index e2d2cfb2..bfd254a4 100644 --- a/test/ForkingManager.t.sol +++ b/test/ForkingManager.t.sol @@ -608,7 +608,7 @@ contract ForkingManagerTest is Test { function testExecuteForkRespectsTime() public { // reverts on empty proposal list - vm.expectRevert("ForkingManager: fork not ready"); + vm.expectRevert(IForkingManager.NotYetReadyToFork.selector); forkmanager.executeFork(); // Mint and approve the arbitration fee for the test contract @@ -621,7 +621,7 @@ contract ForkingManagerTest is Test { vm.warp(testTimestamp); forkmanager.initiateFork(disputeData); - vm.expectRevert("ForkingManager: fork not ready"); + vm.expectRevert(IForkingManager.NotYetReadyToFork.selector); forkmanager.executeFork(); vm.warp(testTimestamp + forkmanager.forkPreparationTime() + 1); forkmanager.executeFork(); @@ -653,7 +653,7 @@ contract ForkingManagerTest is Test { disputeData.disputeContent = "0x1"; forkmanager.initiateFork(disputeData); disputeData.disputeContent = "0x2"; - vm.expectRevert(bytes("ForkingManager: fork pending")); + vm.expectRevert(IForkingManager.ForkingAlreadyInitiated.selector); forkmanager.initiateFork(disputeData); } diff --git a/test/ForkonomicToken.t.sol b/test/ForkonomicToken.t.sol index 285422f0..51717fcb 100644 --- a/test/ForkonomicToken.t.sol +++ b/test/ForkonomicToken.t.sol @@ -57,7 +57,7 @@ contract ForkonomicTokenTest is Test { assertEq(forkonomicToken.balanceOf(address(this)), mintAmount); - vm.expectRevert(bytes("Caller is not a minter")); + vm.expectRevert(IForkonomicToken.NotMinterRole.selector); forkonomicToken.mint(address(this), mintAmount); }