style: use custom errors instead (#56)

* style: use custom errors instead

* fix: structured files for custom errors

* style: change INITIAL values to upper-case

* Update src/Funnel.sol

Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

Co-authored-by: Edison <6057323+edison0x@users.noreply.github.com>
Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

.gitmodules

@@ -3,4 +3,4 @@
 	url = https://github.com/foundry-rs/forge-std
 [submodule "lib/openzeppelin-contracts"]
 	path = lib/openzeppelin-contracts
-	url = https://github.com/openzeppelin/openzeppelin-contracts
+	url = https://github.com/openzeppelin/openzeppelin-contracts

README.md

@@ -11,7 +11,7 @@ Each funnel contract is a proxy/wrapper for an underlying ERC20 token, funneling
 By using this funnel contract, a sender can approve a spender a spending limit every periodic interval. For example, a subscriber can approve a merchant to deduct up to 100 USDC from his account every month.
 
 1. User first approves the funnel contract to spend using ERC20 approvals
-2. User can set renewable allowance on the funnel contract for a given period for an address (spender), approving up to a max limit with a recovery rate. 
+2. User can set renewable allowance on the funnel contract for a given period for an address (spender), approving up to a max limit with a recovery rate.
 3. The spender can then withdraw money out of the user's account up to the available allowance on the account.
 
 What is recovery rate? Recovery rate (amount per second) specifies the rate at which the allowance recovers over time. Once a spender spends the money, the available balance first decreases and slowly restores back to the max limit. Unlike conventional finance apps which performs discrete "resets" of spending limit, we implement renewable allowance using a continuous `recoveryRate` as it allows for more flexible usecases no bound by reset cycles and can be implemented more simply.
@@ -20,7 +20,7 @@ What is recovery rate? Recovery rate (amount per second) specifies the rate at w
 
 The funnel factory is a contract that deploys new funnel contracts, it is the only contract that can create new funnels.
 
-Goal is to deploy a factory onto all supported chains at the same address, and **every chain will produce the same funnel address for the same token address**. 
+Goal is to deploy a factory onto all supported chains at the same address, and **every chain will produce the same funnel address for the same token address**.
 
 ## Contracts
 
@@ -36,12 +36,11 @@ Goal is to deploy a factory onto all supported chains at the same address, and *
 
 `baseToken()` - Returns the address of the underlying token
 
-
 # Usage
 
 ## Testing
 
-Our tests consist of both Foundry tests and hardhat tests. 
+Our tests consist of both Foundry tests and hardhat tests.
 
 `forge test` - Runs the Foundry tests
 
@@ -62,15 +61,16 @@ Deploy to local fork
 
 Deploy factory to goerli
 
-`forge script script/FunnelFactoryDeployer.sol:FunnelFactoryDeployer --rpc-url $GOERLI_RPC_URL --broadcast --verify --etherscan-api-key $ETHERSCAN_API_KEY`  
+`forge script script/FunnelFactoryDeployer.sol:FunnelFactoryDeployer --rpc-url $GOERLI_RPC_URL --broadcast --verify --etherscan-api-key $ETHERSCAN_API_KEY`
 
 Note: Deployment addresses are dependent on deployer's private key, FunnelFactory bytecode and salt used. Multiple deployments will fail.
 
-## Misc 
+## Misc
 
 Run prettier to format the code
+
 ```sh
-npx prettier --write 'src/**/*.sol'  
+npx prettier --write 'src/**/*.sol'
 ```
 
 # Deployments
@@ -81,7 +81,6 @@ npx prettier --write 'src/**/*.sol'
 | Goerli  | FunnelFactory | 0xDd3e9D430D0681Eaa833DbD6B186E7f031f71837 |
 | Goerli  | USDC (funnel) | 0x3d5499808F8082d239a62B5c4876B6ffD23526d5 |
 
+# License
 
-# License 
-
-MIT @ 2022 Suberra
+MIT @ 2022 Suberra

src/Funnel.sol

@@ -13,14 +13,15 @@ import { IERC5827 } from "./interfaces/IERC5827.sol";
 import { IERC5827Proxy } from "./interfaces/IERC5827Proxy.sol";
 import { IERC5827Spender } from "./interfaces/IERC5827Spender.sol";
 import { IERC5827Payable } from "./interfaces/IERC5827Payable.sol";
+import { IFunnelErrors } from "./interfaces/IFunnelErrors.sol";
 import { MetaTxContext } from "./lib/MetaTxContext.sol";
 import { NativeMetaTransaction } from "./lib/NativeMetaTransaction.sol";
 import { MathUtil } from "./lib/MathUtil.sol";
 
 /// @title Funnel contracts for ERC20
 /// @author Zac (zlace0x), zhongfu (zhongfu), Edison (edison0xyz)
 /// @notice This contract is a funnel for ERC20 tokens. It enforces renewable allowances
-contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable {
+contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable, IFunnelErrors {
     using SafeERC20 for IERC20;
 
     //////////////////////////////////////////////////////////////
@@ -51,11 +52,13 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
     ///                        EIP-2612 STORAGE
     //////////////////////////////////////////////////////////////
 
-    // initial_chain_id to be set during initiailisation
-    uint256 internal initial_chain_id;
+    /// INITIAL_CHAIN_ID to be set during initiailisation
+    /// @dev This value will not change
+    uint256 internal INITIAL_CHAIN_ID;
 
-    // initial_domain_separator to be set during initiailisation
-    bytes32 internal initial_domain_separator;
+    // INITIAL_DOMAIN_SEPARATOR to be set during initiailisation
+    /// @dev This value will not change
+    bytes32 internal INITIAL_DOMAIN_SEPARATOR;
 
     // constant for the given struct type that do not need to be runtime computed. Required for EIP712-typed data
     bytes32 internal constant PERMIT_RENEWABLE_TYPEHASH =
@@ -68,13 +71,15 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
         keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");
 
     /// @notice Called when the contract is being initialised.
-    /// @dev Sets the intial_chain_id and initial_domain_separator that might be used in future permit calls
+    /// @dev Sets the INITIAL_CHAIN_ID and INITIAL_DOMAIN_SEPARATOR that might be used in future permit calls
     function initialize(address _token) external initializer {
-        require(_token != address(0), "token address cannot be 0");
+        if (_token == address(0)) {
+            revert InvalidAddress({ _input: _token });
+        }
         _baseToken = IERC20(_token);
 
-        initial_chain_id = block.chainid;
-        initial_domain_separator = computeDomainSeparator();
+        INITIAL_CHAIN_ID = block.chainid;
+        INITIAL_DOMAIN_SEPARATOR = computeDomainSeparator();
     }
 
     /// @dev Fallback function
@@ -130,7 +135,9 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
         bytes32 r,
         bytes32 s
     ) external {
-        require(deadline >= block.timestamp, "PERMIT_DEADLINE_EXPIRED");
+        if (deadline < block.timestamp) {
+            revert PermitExpired();
+        }
 
         uint256 nonce;
         unchecked {
@@ -162,8 +169,9 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
         bytes32 r,
         bytes32 s
     ) external {
-        require(deadline >= block.timestamp, "PERMIT_DEADLINE_EXPIRED");
-
+        if (deadline < block.timestamp) {
+            revert PermitExpired();
+        }
         uint256 nonce;
         unchecked {
             nonce = _nonces[owner]++;
@@ -208,10 +216,9 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
     ) external returns (bool) {
         _approve(_msgSender(), _spender, _value, _recoveryRate);
 
-        require(
-            _checkOnApprovalReceived(_spender, _value, _recoveryRate, data),
-            "IERC5827Payable: IERC5827Spender returned wrong data"
-        );
+        if (!_checkOnApprovalReceived(_spender, _value, _recoveryRate, data)) {
+            revert WrongDataReceivedIERC5827Spender();
+        }
 
         return true;
     }
@@ -245,10 +252,9 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
     ) external returns (bool) {
         transferFrom(from, to, value);
 
-        require(
-            _checkOnTransferReceived(from, to, value, data),
-            "IERC5827Payable: IERC1363Receiver returned wrong data"
-        );
+        if (!_checkOnTransferReceived(from, to, value, data)) {
+            revert WrongDataReceivedIERC1363Receiver();
+        }
         return true;
     }
 
@@ -324,7 +330,7 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
     /// other domains, and satisfy the requirements of EIP-712
     /// @return bytes32 the domain separator
     function DOMAIN_SEPARATOR() public view override returns (bytes32) {
-        return block.chainid == initial_chain_id ? initial_domain_separator : computeDomainSeparator();
+        return block.chainid == INITIAL_CHAIN_ID ? INITIAL_DOMAIN_SEPARATOR : computeDomainSeparator();
     }
 
     /// =================================================================
@@ -365,7 +371,7 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
         bytes memory data
     ) internal returns (bool) {
         if (!Address.isContract(recipient)) {
-            revert("IERC5827Payable: transfer to non contract address");
+            revert NotContractError();
         }
 
         try
@@ -379,7 +385,8 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
             return retval == IERC1363Receiver.onTransferReceived.selector;
         } catch (bytes memory reason) {
             if (reason.length == 0) {
-                revert("IERC5827Payable: transfer to non IERC1363Receiver implementer");
+                // Attempted to transfer to a non-IERC1363Receiver implementer
+                revert NotIERC1363Receiver();
             } else {
                 /// @solidity memory-safe-assembly
                 assembly {
@@ -412,7 +419,7 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
         bytes memory data
     ) internal returns (bool) {
         if (!Address.isContract(_spender)) {
-            revert("IERC5827Payable: approve a non contract address");
+            revert NotContractError();
         }
 
         try IERC5827Spender(_spender).onRenewableApprovalReceived(_msgSender(), _value, _recoveryRate, data) returns (
@@ -421,7 +428,8 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable
             return retval == IERC5827Spender.onRenewableApprovalReceived.selector;
         } catch (bytes memory reason) {
             if (reason.length == 0) {
-                revert("IERC5827Payable: approve a non IERC5827Spender implementer");
+                // attempting to approve a non IERC5827Spender implementer
+                revert NotIERC5827Spender();
             } else {
                 /// @solidity memory-safe-assembly
                 assembly {

src/FunnelFactory.sol

@@ -3,13 +3,14 @@ pragma solidity 0.8.17;
 
 import { IFunnelFactory } from "./interfaces/IFunnelFactory.sol";
 import { IERC5827Proxy } from "./interfaces/IERC5827Proxy.sol";
+import { IFunnelErrors } from "./interfaces/IFunnelErrors.sol";
 import { Funnel } from "./Funnel.sol";
 import { Clones } from "openzeppelin-contracts/proxy/Clones.sol";
 
 /// @title Factory for all the funnel contracts
 /// @author Zac (zlace0x), zhongfu (zhongfu), Edison (edison0xyz)
 
-contract FunnelFactory is IFunnelFactory {
+contract FunnelFactory is IFunnelFactory, IFunnelErrors {
     using Clones for address;
 
     /// Stores the mapping between tokenAddress => funnelAddress
@@ -23,7 +24,9 @@ contract FunnelFactory is IFunnelFactory {
     /// @dev requires a valid funnelImplementation address
     /// @param _funnelImplementation The address of the implementation
     constructor(address _funnelImplementation) {
-        require(_funnelImplementation != address(0), "implementation cannot be zero");
+        if (_funnelImplementation == address(0)) {
+            revert InvalidAddress({ _input: _funnelImplementation });
+        }
         funnelImplementation = _funnelImplementation;
     }
 

src/interfaces/IFunnel.sol

@@ -8,6 +8,18 @@ import { IERC5827Proxy } from "./IERC5827Proxy.sol";
 /// @title Interface for Funnel contracts for ERC20
 /// @author Zac (zlace0x), zhongfu (zhongfu), Edison (edison0xyz)
 interface IFunnel is IERC5827, IERC5827Proxy, IERC5827Payable {
-    // Error thrown if the Recovery Rate exceeds the max allowance
+    /// @dev Wrong data received from IERC1363 Receiver
+    error WrongDataReceivedIERC1363Receiver();
+
+    /// @dev Error thrown when attempting to transfer to a non IERC1363Receiver
+    error NotIERC1363Receiver();
+
+    /// @dev Wrong data received from IERC5827Spender
+    error WrongDataReceivedIERC5827Spender();
+
+    /// @dev Error thrown when attempting to transfer to a non IERC5827Spender
+    error NotIERC5827Spender();
+
+    /// @dev Error thrown if the Recovery Rate exceeds the max allowance
     error RecoveryRateExceeded();
 }

src/interfaces/IFunnelErrors.sol

@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.17;
+
+/// @title Shared errors for Funnel Contracts and FunnelFactory
+/// @author Zac (zlace0x), zhongfu (zhongfu), Edison (edison0xyz)
+interface IFunnelErrors {
+    /// @dev Invalid address, could be due to zero address
+    /// @param _input address that caused the error.
+    error InvalidAddress(address _input);
+
+    /// Error thrown when the token is invalid
+    error InvalidToken();
+
+    /// @dev Thrown when attempting to interact with a non-contract.
+    error NotContractError();
+
+    /// @dev Error thrown when the permit deadline expires
+    error PermitExpired();
+}

src/interfaces/IFunnelFactory.sol

@@ -4,15 +4,14 @@ pragma solidity 0.8.17;
 /// @title Interface for Funnel Factory
 /// @author Zac (zlace0x), zhongfu (zhongfu), Edison (edison0xyz)
 interface IFunnelFactory {
+    /// ==== Factory Errors =====
+
     /// Error thrown when funnel is not deployed
     error FunnelNotDeployed();
 
     /// Error thrown when funnel is already deployed.
     error FunnelAlreadyDeployed();
 
-    /// Error thrown when the token is invalid
-    error InvalidToken();
-
     /// @notice Event emitted when the funnel contract is deployed
     /// @param tokenAddress of the base token (indexed)
     /// @param funnelAddress of the deployed funnel contract (indexed)

test/Funnel.t.sol

@@ -4,7 +4,7 @@ pragma solidity 0.8.17;
 import "forge-std/Test.sol";
 import { ERC20PresetFixedSupply, ERC20 } from "openzeppelin-contracts/token/ERC20/presets/ERC20PresetFixedSupply.sol";
 import { IERC20Metadata } from "openzeppelin-contracts/interfaces/IERC20Metadata.sol";
-import { Funnel, IFunnel } from "../src/Funnel.sol";
+import { Funnel, IFunnel, IFunnelErrors } from "../src/Funnel.sol";
 import { ERC5827TestSuite } from "./ERC5827TestSuite.sol";
 import { MockSpenderReceiver } from "../src/mocks/MockSpenderReceiver.sol";
 
@@ -137,7 +137,7 @@ contract FunnelTest is ERC5827TestSuite {
         vm.prank(user1);
         funnel.approveRenewable(user2, 1337, 1);
         vm.prank(user2);
-        vm.expectRevert("IERC5827Payable: transfer to non contract address");
+        vm.expectRevert(IFunnelErrors.NotContractError.selector);
         funnel.transferFromAndCall(user1, address(user3), 1337, "");
     }
 
@@ -146,7 +146,8 @@ contract FunnelTest is ERC5827TestSuite {
         funnel.approveRenewable(user2, 1337, 1);
 
         vm.prank(user2);
-        vm.expectRevert("IERC5827Payable: transfer to non IERC1363Receiver implementer");
+        // Attempting to transfer to a non IERC1363Receiver
+        vm.expectRevert(IFunnel.NotIERC1363Receiver.selector);
         funnel.transferFromAndCall(user1, address(token), 1337, "");
     }
 
@@ -158,12 +159,13 @@ contract FunnelTest is ERC5827TestSuite {
     }
 
     function testApproveRenewableAndCallRevertNonContract() public {
-        vm.expectRevert("IERC5827Payable: approve a non contract address");
+        vm.expectRevert(IFunnelErrors.NotContractError.selector);
         funnel.approveRenewableAndCall(address(user3), 1337, 1, "");
     }
 
     function testApproveRenewableAndCallRevertNonReceiver() public {
-        vm.expectRevert("IERC5827Payable: approve a non IERC5827Spender implementer");
+        // attempting to approve a non IERC5827Spender
+        vm.expectRevert(IFunnel.NotIERC5827Spender.selector);
         funnel.approveRenewableAndCall(address(token), 1337, 1, "");
     }
 

test/FunnelFactory.t.sol

@@ -4,7 +4,7 @@ pragma solidity 0.8.17;
 import { console } from "forge-std/console.sol";
 import "forge-std/Test.sol";
 import { ERC20PresetFixedSupply, ERC20 } from "openzeppelin-contracts/token/ERC20/presets/ERC20PresetFixedSupply.sol";
-import { FunnelFactory } from "../src/FunnelFactory.sol";
+import { FunnelFactory, IFunnelErrors } from "../src/FunnelFactory.sol";
 import { Funnel } from "../src/Funnel.sol";
 import { IFunnelFactory } from "../src/interfaces/IFunnelFactory.sol";
 import { Clones } from "openzeppelin-contracts/proxy/Clones.sol";
@@ -87,7 +87,7 @@ contract FunnelFactoryTest is Test {
     }
 
     function testNoCodeTokenReverts() public {
-        vm.expectRevert(IFunnelFactory.InvalidToken.selector);
+        vm.expectRevert(IFunnelErrors.InvalidToken.selector);
         funnelFactory.deployFunnelForToken(tokenAddress3);
     }