fix: use custom errors to reduce gas consumption (#187)

Co-authored-by: Oleksii Matiiasevych <[email protected]>
sygmaprotocol · May 8, 2023 · 51a2eb8 · 51a2eb8
1 parent 84732d0
commit 51a2eb8
Show file tree

Hide file tree

Showing 30 changed files with 429 additions and 174 deletions.
diff --git a/contracts/Bridge.sol b/contracts/Bridge.sol
@@ -79,13 +79,33 @@ contract Bridge is Pausable, Context, EIP712 {
 
     event Retry(string txHash);
 
+    error AccessNotAllowed(address sender, bytes4 funcSig);
+
+    error ResourceIDNotMappedToHandler();
+
+    error DepositToCurrentDomain();
+
+    error InvalidProposalSigner();
+
+    error EmptyProposalsArray();
+
+    error NonceDecrementsNotAllowed();
+
+    error MPCAddressAlreadySet();
+
+    error MPCAddressNotSet();
+
+    error MPCAddressIsNotUpdatable();
+
+    error MPCAddressZeroAddress();
+
     modifier onlyAllowed() {
         _onlyAllowed(msg.sig, _msgSender());
         _;
     }
 
     function _onlyAllowed(bytes4 sig, address sender) private view {
-        require(_accessControl.hasAccess(sig, sender), "sender doesn't have access to function");
+        if (!_accessControl.hasAccess(sig, sender)) revert AccessNotAllowed(sender, sig);
     }
 
     function _msgSender() internal override view returns (address) {
@@ -127,7 +147,7 @@ contract Bridge is Pausable, Context, EIP712 {
         @notice MPC address has to be set before Bridge can be unpaused
      */
     function adminUnpauseTransfers() external onlyAllowed {
-        require(_MPCAddress != address(0), "MPC address not set");
+        if (_MPCAddress == address(0)) revert MPCAddressNotSet();
         _unpause(_msgSender());
     }
 
@@ -228,16 +248,16 @@ contract Bridge is Pausable, Context, EIP712 {
         @param feeData Additional data to be passed to the fee handler.
         @notice Emits {Deposit} event with all necessary parameters and a handler response.
         @return depositNonce deposit nonce for the destination domain.
-        @return handlerResponse a handler response: 
+        @return handlerResponse a handler response:
         - ERC20Handler: responds with an empty data.
         - ERC721Handler: responds with the deposited token metadata acquired by calling a tokenURI method in the token contract.
         - PermissionedGenericHandler: responds with the raw bytes returned from the call to the target contract.
         - PermissionlessGenericHandler: responds with an empty data.
      */
-    function deposit(uint8 destinationDomainID, bytes32 resourceID, bytes calldata depositData, bytes calldata feeData) 
-        external payable whenNotPaused 
+    function deposit(uint8 destinationDomainID, bytes32 resourceID, bytes calldata depositData, bytes calldata feeData)
+        external payable whenNotPaused
         returns (uint64 depositNonce, bytes memory handlerResponse) {
-        require(destinationDomainID != _domainID, "Can't deposit to current domain");
+        if (destinationDomainID == _domainID) revert DepositToCurrentDomain();
 
         address sender = _msgSender();
         if (address(_feeHandler) == address(0)) {
@@ -247,7 +267,7 @@ contract Bridge is Pausable, Context, EIP712 {
             _feeHandler.collectFee{value: msg.value}(sender, _domainID, destinationDomainID, resourceID, depositData, feeData);
         }
         address handler = _resourceIDToHandlerAddress[resourceID];
-        require(handler != address(0), "resourceID not mapped to handler");
+        if (handler == address(0)) revert ResourceIDNotMappedToHandler();
 
         depositNonce = ++_depositCounts[destinationDomainID];
 
@@ -294,8 +314,8 @@ contract Bridge is Pausable, Context, EIP712 {
         In the case of {PermissionedGenericHandler}, when execution fails, the handler will emit a failure event and terminate the function normally.
      */
     function executeProposals(Proposal[] memory proposals, bytes calldata signature) public whenNotPaused {
-        require(proposals.length > 0, "Proposals can't be an empty array");
-        require(verify(proposals, signature), "Invalid proposal signer");
+        if (proposals.length == 0) revert EmptyProposalsArray();
+        if (!verify(proposals, signature)) revert InvalidProposalSigner();
 
         for (uint256 i = 0; i < proposals.length; i++) {
             if(isProposalExecuted(proposals[i].originDomainID, proposals[i].depositNonce)) {
@@ -327,7 +347,7 @@ contract Bridge is Pausable, Context, EIP712 {
         which is mapped in {functionAccess} in AccessControlSegregator contract.
      */
     function startKeygen() external onlyAllowed {
-        require(_MPCAddress == address(0), "MPC address is already set");
+        if (_MPCAddress != address(0)) revert MPCAddressAlreadySet();
         emit StartKeygen();
     }
 
@@ -339,8 +359,8 @@ contract Bridge is Pausable, Context, EIP712 {
         @param MPCAddress Address that will be set as MPC address.
      */
     function endKeygen(address MPCAddress) external onlyAllowed {
-        require(MPCAddress != address(0), "MPC address can't be null-address");
-        require(_MPCAddress == address(0), "MPC address can't be updated");
+        if( MPCAddress == address(0)) revert MPCAddressZeroAddress();
+        if (_MPCAddress != address(0)) revert MPCAddressIsNotUpdatable();
         _MPCAddress = MPCAddress;
         _unpause(_msgSender());
         emit EndKeygen();

diff --git a/contracts/ERC20Safe.sol b/contracts/ERC20Safe.sol
@@ -101,5 +101,4 @@ contract ERC20Safe {
             require(abi.decode(returndata, (bool)), "ERC20: operation did not succeed");
         }
     }
-
 }
diff --git a/contracts/TestContracts.sol b/contracts/TestContracts.sol
@@ -2,7 +2,6 @@
 // SPDX-License-Identifier: BUSL-1.1
 pragma solidity 0.8.11;
 
-import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Wrapper.sol";
 import "@openzeppelin/contracts/token/ERC20/presets/ERC20PresetMinterPauser.sol";
 import "./handlers/ERCHandlerHelpers.sol";
 import "./interfaces/IERC20Plus.sol";
@@ -160,6 +159,37 @@ contract XC20Test is ERC20 {
     }
 }
 
+/**
+  @dev This contract mocks XC20Test where "transferFrom()" always fails
+ */
+contract XC20TestMock is XC20Test {
+
+    function transferFrom(address from, address to, uint256 amount) public virtual override(ERC20) returns (bool) {
+        address spender = _msgSender();
+        _spendAllowance(from, spender, amount);
+        _transfer(from, to, amount);
+        return false;
+    }
+}
+
+/**
+  @dev This contract mocks ERC20PresetMinterPauser where and "transferFrom()" always fails
+ */
+contract ERC20PresetMinterPauserMock is ERC20PresetMinterPauser {
+
+    constructor(
+        string memory name,
+        string memory symbol
+    ) ERC20PresetMinterPauser(name, symbol) {}
+
+    function transferFrom(address from, address to, uint256 amount) public virtual override(ERC20) returns (bool) {
+        address spender = _msgSender();
+        _spendAllowance(from, spender, amount);
+        _transfer(from, to, amount);
+        return false;
+    }
+}
+
 contract ERC20PresetMinterPauserDecimals is ERC20PresetMinterPauser {
 
     uint8 private immutable customDecimals;

diff --git a/contracts/handlers/ERC1155Handler.sol b/contracts/handlers/ERC1155Handler.sol
@@ -77,7 +77,7 @@ contract ERC1155Handler is IHandler, ERCHandlerHelpers, ERC1155Safe, ERC1155Hold
         }
 
         address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
-        require(_tokenContractAddressToTokenProperties[address(tokenAddress)].isWhitelisted, "provided tokenAddress is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);
 
         if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
             mintBatchERC1155(tokenAddress, address(recipientAddress), tokenIDs, amounts, transferData);

diff --git a/contracts/handlers/ERC20Handler.sol b/contracts/handlers/ERC20Handler.sol
@@ -42,7 +42,7 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, ERC20Safe {
         (amount) = abi.decode(data, (uint));
 
         address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
-        require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);
 
         if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
             burnERC20(tokenAddress, depositor, amount);
@@ -79,7 +79,7 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, ERC20Safe {
             recipientAddress := mload(add(destinationRecipientAddress, 0x20))
         }
 
-        require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);
 
         if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
             mintERC20(tokenAddress, address(recipientAddress), convertToExternalBalance(tokenAddress, amount));

diff --git a/contracts/handlers/ERC721Handler.sol b/contracts/handlers/ERC721Handler.sol
@@ -54,7 +54,7 @@ contract ERC721Handler is IHandler, ERCHandlerHelpers, ERC721Safe {
         (tokenID) = abi.decode(data, (uint));
 
         address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
-        require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);
 
         // Check if the contract supports metadata, fetch it if it does
         if (tokenAddress.supportsInterface(_INTERFACE_ERC721_METADATA)) {
@@ -103,7 +103,7 @@ contract ERC721Handler is IHandler, ERCHandlerHelpers, ERC721Safe {
         }
 
         address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
-        require(_tokenContractAddressToTokenProperties[address(tokenAddress)].isWhitelisted, "provided tokenAddress is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);
 
         if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
             mintERC721(tokenAddress, address(recipientAddress), tokenID, metaData);

diff --git a/contracts/handlers/ERCHandlerHelpers.sol b/contracts/handlers/ERCHandlerHelpers.sol
@@ -26,6 +26,8 @@ contract ERCHandlerHelpers is IERCHandler {
       Decimals decimals;
     }
 
+    error ContractAddressNotWhitelisted(address contractAddress);
+
     // resourceID => token contract address
     mapping (bytes32 => address) public _resourceIDToTokenContractAddress;
 
@@ -71,7 +73,7 @@ contract ERCHandlerHelpers is IERCHandler {
     }
 
     function _setBurnable(address contractAddress) internal {
-        require(_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted, "provided contract is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted) revert ContractAddressNotWhitelisted(contractAddress);
         _tokenContractAddressToTokenProperties[contractAddress].isBurnable = true;
     }
 
@@ -83,7 +85,7 @@ contract ERCHandlerHelpers is IERCHandler {
         @param externalDecimals Decimal places of token that is transferred.
      */
     function _setDecimals(address contractAddress, uint8 externalDecimals) internal {
-        require(_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted, "provided contract is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted) revert ContractAddressNotWhitelisted(contractAddress);
         _tokenContractAddressToTokenProperties[contractAddress].decimals = Decimals({
             isSet: true,
             externalDecimals: externalDecimals

diff --git a/contracts/handlers/PermissionedGenericHandler.sol b/contracts/handlers/PermissionedGenericHandler.sol
@@ -20,6 +20,8 @@ contract PermissionedGenericHandler is IHandler {
       bool isWhitelisted;
     }
 
+    error ContractAddressNotWhitelisted(address contractAddress);
+
     // token contract address => TokenContractProperties
     mapping (address => TokenContractProperties) public _tokenContractAddressToTokenProperties;
 
@@ -106,7 +108,7 @@ contract PermissionedGenericHandler is IHandler {
             require(depositor == address(uint160(metadataDepositor >> 96)), 'incorrect depositor in the data');
         }
 
-        require(_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted, "provided contractAddress is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted) revert ContractAddressNotWhitelisted(contractAddress);
 
         bytes4 sig = _tokenContractAddressToTokenProperties[contractAddress].depositFunctionSignature;
         if (sig != bytes4(0)) {
@@ -136,7 +138,7 @@ contract PermissionedGenericHandler is IHandler {
         metaData = bytes(data[32:32 + lenMetadata]);
 
         address contractAddress = _resourceIDToContractAddress[resourceID];
-        require(_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted, "provided contractAddress is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted) revert ContractAddressNotWhitelisted(contractAddress);
 
         bytes4 sig = _tokenContractAddressToTokenProperties[contractAddress].executeFunctionSignature;
         if (sig != bytes4(0)) {

diff --git a/contracts/handlers/XC20Handler.sol b/contracts/handlers/XC20Handler.sol
@@ -42,7 +42,7 @@ contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
         (amount) = abi.decode(data, (uint));
 
         address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
-        require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);
 
         if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
             burnERC20(tokenAddress, depositor, amount);
@@ -79,7 +79,7 @@ contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
             recipientAddress := mload(add(destinationRecipientAddress, 0x20))
         }
 
-        require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
+        if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);
 
         if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
             mintERC20(tokenAddress, address(recipientAddress), convertToExternalBalance(tokenAddress, amount));

diff --git a/contracts/handlers/fee/BasicFeeHandler.sol b/contracts/handlers/fee/BasicFeeHandler.sol
@@ -20,6 +20,8 @@ contract BasicFeeHandler is IFeeHandler, AccessControl {
         uint256 newFee
     );
 
+    error IncorrectFeeSupplied(uint256);
+
     modifier onlyAdmin() {
         require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "sender doesn't have admin role");
         _;
@@ -69,7 +71,7 @@ contract BasicFeeHandler is IFeeHandler, AccessControl {
         @param feeData Additional data to be passed to the fee handler.
      */
     function collectFee(address sender, uint8 fromDomainID, uint8 destinationDomainID, bytes32 resourceID, bytes calldata depositData, bytes calldata feeData) payable external onlyBridgeOrRouter {
-        require(msg.value == _fee, "Incorrect fee supplied");
+        if (msg.value != _fee) revert IncorrectFeeSupplied(msg.value);
         emit FeeCollected(sender, fromDomainID, destinationDomainID, resourceID, _fee, address(0));
     }
 

diff --git a/contracts/handlers/fee/DynamicERC20FeeHandlerEVM.sol b/contracts/handlers/fee/DynamicERC20FeeHandlerEVM.sol
@@ -57,7 +57,7 @@ contract DynamicERC20FeeHandlerEVM is DynamicFeeHandler {
             total: 353
         */
 
-        require(feeData.length == 353, "Incorrect feeData length");
+        if (feeData.length != 353) revert IncorrectFeeDataLength(feeData.length);
 
         FeeDataType memory feeDataDecoded;
         uint256 txCost;
@@ -66,12 +66,18 @@ contract DynamicERC20FeeHandlerEVM is DynamicFeeHandler {
         feeDataDecoded.sig = bytes(feeData[256: 321]);
 
         OracleMessageType memory oracleMessage = abi.decode(feeDataDecoded.message, (OracleMessageType));
-        require(block.timestamp <= oracleMessage.expiresAt, "Obsolete oracle data");
-        require((oracleMessage.fromDomainID == fromDomainID)
-            && (oracleMessage.toDomainID == destinationDomainID)
-            && (oracleMessage.resourceID == resourceID),
-            "Incorrect deposit params"
-        );
+        if (block.timestamp > oracleMessage.expiresAt) revert ObsoleteOracleData();
+        if ((oracleMessage.fromDomainID != fromDomainID) ||
+            (oracleMessage.toDomainID != destinationDomainID) ||
+            (oracleMessage.resourceID != resourceID)
+        ) {
+            revert IncorrectDepositParams(
+                oracleMessage.fromDomainID,
+                oracleMessage.toDomainID,
+                oracleMessage.resourceID
+            );
+        }
+
 
         bytes32 messageHash = keccak256(feeDataDecoded.message);
 

diff --git a/contracts/handlers/fee/DynamicERC20FeeHandlerSubstrate.sol b/contracts/handlers/fee/DynamicERC20FeeHandlerSubstrate.sol
@@ -71,7 +71,7 @@ contract DynamicERC20FeeHandlerSubstrate is DynamicFeeHandler {
             total: 353
         */
 
-        require(feeData.length == 353, "Incorrect feeData length");
+        if (feeData.length != 353) revert IncorrectFeeDataLength(feeData.length);
 
         FeeDataType memory feeDataDecoded;
         uint256 txCost;
@@ -80,12 +80,17 @@ contract DynamicERC20FeeHandlerSubstrate is DynamicFeeHandler {
         feeDataDecoded.sig = bytes(feeData[256: 321]);
 
         SubstrateOracleMessageType memory oracleMessage = abi.decode(feeDataDecoded.message, (SubstrateOracleMessageType));
-        require(block.timestamp <= oracleMessage.expiresAt, "Obsolete oracle data");
-        require((oracleMessage.fromDomainID == fromDomainID)
-            && (oracleMessage.toDomainID == destinationDomainID)
-            && (oracleMessage.resourceID == resourceID),
-            "Incorrect deposit params"
-        );
+        if (block.timestamp > oracleMessage.expiresAt) revert ObsoleteOracleData();
+        if ((oracleMessage.fromDomainID != fromDomainID) ||
+            (oracleMessage.toDomainID != destinationDomainID) ||
+            (oracleMessage.resourceID != resourceID)
+        ) {
+            revert IncorrectDepositParams(
+                oracleMessage.fromDomainID,
+                oracleMessage.toDomainID,
+                oracleMessage.resourceID
+            );
+        }
 
         bytes32 messageHash = keccak256(feeDataDecoded.message);
 

diff --git a/contracts/handlers/fee/DynamicFeeHandler.sol b/contracts/handlers/fee/DynamicFeeHandler.sol
@@ -47,6 +47,16 @@ abstract contract DynamicFeeHandler is IFeeHandler, AccessControl, ERC20Safe {
 
     event FeeOraclePropertiesSet(uint32 gasUsed, uint16 feePercent);
 
+    error InvalidSignature();
+
+    error IncorrectFeeDataLength(uint256);
+
+    error IncorrectFeeSupplied(uint256);
+
+    error ObsoleteOracleData();
+
+    error IncorrectDepositParams(uint8 fromDomainID, uint8 destinationDomainID, bytes32 resourceID);
+
     modifier onlyAdmin() {
         require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "sender doesn't have admin role");
         _;
@@ -121,7 +131,7 @@ abstract contract DynamicFeeHandler is IFeeHandler, AccessControl, ERC20Safe {
     function collectFee(address sender, uint8 fromDomainID, uint8 destinationDomainID, bytes32 resourceID, bytes calldata depositData, bytes calldata feeData) payable external onlyBridgeOrRouter {
         (uint256 fee, address tokenAddress) = _calculateFee(sender, fromDomainID, destinationDomainID, resourceID, depositData, feeData);
         if(tokenAddress == address(0)){
-            require(msg.value == fee, "Incorrect fee supplied");
+            if (msg.value != fee) revert IncorrectFeeSupplied(msg.value);
         } else {
             require(msg.value == 0, "collectFee: msg.value != 0");
             lockERC20(tokenAddress, sender, address(this), fee);
@@ -166,6 +176,6 @@ abstract contract DynamicFeeHandler is IFeeHandler, AccessControl, ERC20Safe {
 
     function verifySig(bytes32 message, bytes memory signature, address signerAddress) internal pure {
         address signerAddressRecovered = ECDSA.recover(message, signature);
-        require(signerAddressRecovered == signerAddress, 'Invalid signature');
+        if (signerAddressRecovered != signerAddress) revert InvalidSignature();
     }
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -101,5 +101,4 @@ contract ERC20Safe { @@
                 require(abi.decode(returndata, (bool)), "ERC20: operation did not succeed");
             }
         }
     }