Merge pull request #27 from spherex-collab/M05

M05

README.md

@@ -123,26 +123,49 @@ graph TD
 
 ## Notes regarding SphereXProtected contract
 
-This is an abstract contract with 2 state variable, but to make sure they do not interrupt the layout of the inherited contract's variables, they are actually saved in distinct storage slots. This is why these are not normal state variables with names. These 2 special addresses contain: 
+This is an abstract contract with 4 state variable, but to make sure they do not interrupt the layout of the inherited contract's variables, they are actually saved in distinct storage slots. This is why these are not normal state variables with names. These 4 special addresses contain: 
 
 1. The address of the SphereXAdmin
-   - This account is the only one who can change the address of the ENGINE state variable, and pass it's role to another address.
-2. The address of the SphereXEngine - this is the address to which data is sent, and which classifies the transaction (at various points of execution).
+   - This account is the only one who can change the address of the OPERTOR state variable, and pass it's role to another address.
+2. the address of the pendingSphereXAdmin
+   - This stte variable is used to implement a two step admin role transfer.
+3. the address of the sphereXOperator
+   - This account is the only onw responsible for managing the protected contract, including changing the address of the ENGINE. 
+4. The address of the SphereXEngine - this is the address to which data is sent, and which classifies the transaction (at various points of execution).
     - If this value is `address(0)`, the engine will be bypassed, essentially disabling its protection.
 
 
-The contract provides functions to transfer the role of the SphereXAdmin, and update the address of the engine.
+The contract provides functions to transfer the role of the SphereXAdmin, SphereXOperator, and update the address of the engine.
 
 The heart of the contract is its modifiers - `SphereXProtected` provides 3 different modifiers to be used on functions with different visibilities:
-1. `sphereXGuardInternal(int16 num)` - used for internal/private functions. The number that should be provided is an id to the function. It should be positive, and we simply use simple counting int starting with 1.
-2. `sphereXGuardExternal(int16 num)` - used for external functions. The number has the same meaning and usage as in the internal.
-3. `sphereXGuardPublic(int16 num, bytes4 selector)` - used fo public functions. The first param is just like in the previous 2 modifiers, and the `selector` param is the 4 bytes selector of the function.
+1. `sphereXGuardInternal(int256 num)` - used for internal/private functions. The number that should be provided is an id to the function. It should be positive, and we simply use simple counting int starting with 1.
+2. `sphereXGuardExternal(int256 num)` - used for external functions. The number has the same meaning and usage as in the internal.
+3. `sphereXGuardPublic(int256 num, bytes4 selector)` - used fo public functions. The first param is just like in the previous 2 modifiers, and the `selector` param is the 4 bytes selector of the function.
 
+### SphereXProtectedBase
+The only difference between SphereXProtectedBase and SphereXProtected is the constructor and the init function, the SphereXProtectedBase has explicit initialization process (receives parameters), and the SphereXProtected has an implicit initialization process (initializes the admin to msg.sender and the rest to address(0)).
+
+### Factory integration note
+When integrating the protected contract with a factory pattern there a few key points to look at:
+1. The contract created by the factory should inherit from SphereXProtectedBase
+2. Upon creating the new contract the process should look something like:
+
+```
+ChildContract newChild = new ChildContract(..., sphereXAdmin(), sphereXOperator(), sphereXEngine());
+_addAllowedSenderOnChain(address(newChild));
+```
 
 ## Notes regarding SphereXEngine contract
 
-This contract is the brain of the system. It is managed by an owner account (we have stripped down and simplified the OpenZeppelin `Ownable` template).
+This contract is the brain of the system. It is managed by an AccessControlDefaultAdminRules pattern (inherited from OpenZeppelin).
 
+The engine has 3 key roles:
+1. The owner
+   - Controls the operator role address.
+2. The operator
+   - Controls the engine managment functions.
+3. The sender adder
+   - A unique role granted to mainly on-chain factory contracts, its sole purpose is to allow adding an address to the `allowedSenders`.
 It has 3 important state variables:
 
 1. `_engineRules` - a bytes8 variable indicating what rules (theses) are applied, 1 for CF, 2 for PrefixTxFlow, if all are off then the engine is deactivated.

gas-report

@@ -1,21 +1,23 @@
 | src/SphereXEngine.sol:SphereXEngine contract |                 |       |        |       |         |
 |----------------------------------------------|-----------------|-------|--------|-------|---------|
 | Deployment Cost                              | Deployment Size |       |        |       |         |
-| 1847252                                      | 10391           |       |        |       |         |
+| 1912323                                      | 10716           |       |        |       |         |
 | Function Name                                | min             | avg   | median | max   | # calls |
 | OPERATOR_ROLE                                | 338             | 338   | 338    | 338   | 3       |
+| SENDER_ADDER_ROLE                            | 307             | 307   | 307    | 307   | 6       |
 | acceptDefaultAdminTransfer                   | 27892           | 27892 | 27892  | 27892 | 1       |
-| addAllowedPatterns                           | 5406            | 24458 | 25306  | 48304 | 123     |
-| addAllowedSender                             | 1007            | 24848 | 25351  | 25351 | 93      |
-| beginDefaultAdminTransfer                    | 12095           | 12095 | 12095  | 12095 | 1       |
-| configureRules                               | 2352            | 22409 | 24252  | 26252 | 96      |
-| deactivateAllRules                           | 2705            | 5308  | 6176   | 6176  | 4       |
-| grantRole                                    | 29689           | 31303 | 29689  | 34533 | 3       |
-| removeAllowedPatterns                        | 2777            | 3363  | 3656   | 3656  | 3       |
-| removeAllowedSender                          | 3006            | 5402  | 6600   | 6600  | 3       |
+| addAllowedPatterns                           | 5406            | 24670 | 25306  | 48304 | 145     |
+| addAllowedSender                             | 1007            | 24888 | 25351  | 25351 | 101     |
+| addAllowedSenderOnchain                      | 2814            | 20709 | 26675  | 26675 | 8       |
+| beginDefaultAdminTransfer                    | 12117           | 12117 | 12117  | 12117 | 1       |
+| configureRules                               | 2352            | 22551 | 24252  | 26252 | 104     |
+| deactivateAllRules                           | 2727            | 5330  | 6198   | 6198  | 4       |
+| grantRole                                    | 29623           | 30161 | 29623  | 34467 | 9       |
+| removeAllowedPatterns                        | 2725            | 3311  | 3604   | 3604  | 3       |
+| removeAllowedSender                          | 2961            | 5363  | 6564   | 6564  | 3       |
 | revokeRole                                   | 2948            | 2948  | 2948   | 2948  | 1       |
-| sphereXValidateInternalPost                  | 458             | 2129  | 1742   | 3677  | 79      |
-| sphereXValidateInternalPre                   | 434             | 4161  | 1962   | 8847  | 86      |
-| sphereXValidatePost                          | 992             | 3147  | 2659   | 4659  | 86      |
-| sphereXValidatePre                           | 926             | 5823  | 2449   | 11334 | 94      |
-| supportsInterface                            | 435             | 435   | 435    | 435   | 49      |
+| sphereXValidateInternalPost                  | 972             | 2640  | 2254   | 4181  | 79      |
+| sphereXValidateInternalPre                   | 653             | 4373  | 2178   | 9063  | 86      |
+| sphereXValidatePost                          | 992             | 3128  | 2659   | 4659  | 96      |
+| sphereXValidatePre                           | 948             | 6134  | 2513   | 11356 | 106     |
+| supportsInterface                            | 435             | 435   | 435    | 435   | 65      |

src/ISphereXEngine.sol

@@ -26,4 +26,6 @@ interface ISphereXEngine is IERC165 {
         bytes32[] calldata valuesBefore,
         bytes32[] calldata valuesAfter
     ) external;
+
+    function addAllowedSenderOnChain(address sender) external;
 }

src/SphereXEngine.sol

@@ -32,7 +32,11 @@ contract SphereXEngine is ISphereXEngine, AccessControlDefaultAdminRules {
     bytes32 internal constant DEACTIVATED = bytes32(0);
     uint64 internal constant RULES_1_AND_2_TOGETHER = 3;
 
+    // the index of the addAllowedSenderOnChain in the call flow
+    int256 internal constant ADD_ALLOWED_SENDER_ONCHAIN_INDEX = int256(uint256(keccak256("factory.allowed.sender")));
+
     bytes32 public constant OPERATOR_ROLE = keccak256("OPERATOR_ROLE");
+    bytes32 public constant SENDER_ADDER_ROLE = keccak256("SENDER_ADDER_ROLE");
 
     constructor() AccessControlDefaultAdminRules(1 days, msg.sender) {
         grantRole(OPERATOR_ROLE, msg.sender);
@@ -43,9 +47,15 @@ contract SphereXEngine is ISphereXEngine, AccessControlDefaultAdminRules {
         _;
     }
 
+    modifier onlySenderAdderRole() {
+        require(hasRole(SENDER_ADDER_ROLE, msg.sender), "SphereX error: sender adder required");
+        _;
+    }
+
     event TxStartedAtIrregularDepth();
     event ConfigureRules(bytes8 oldRules, bytes8 newRules);
     event AddedAllowedSenders(address[] senders);
+    event AddedAllowedSenderOnchain(address sender);
     event RemovedAllowedSenders(address[] senders);
     event AddedAllowedPatterns(uint216[] patterns);
     event RemovedAllowedPatterns(uint216[] patterns);
@@ -108,6 +118,23 @@ contract SphereXEngine is ISphereXEngine, AccessControlDefaultAdminRules {
         emit AddedAllowedSenders(senders);
     }
 
+    /**
+     * Adds address that will be served by this engine. An address that was never added will get a revert if it tries to call the engine.
+     * @param sender address to add to the set of allowed addresses
+     * @notice This function adds elements to the current pattern in order to guard itself from unwanted calls.
+     * Lets say the client has a contract with SENDER_ADDER role and we approve only function indexed 1 to call addAllowedSenderOnChain.
+     * We will allow the pattern [1, addAllowedSenderOnChain, -addAllowedSenderOnChain ,-1] and by doing so we guarantee no other function
+     * will call addAllowedSenderOnChain.
+     */
+    function addAllowedSenderOnChain(address sender) external returnsIfNotActivated onlySenderAdderRole {
+        _addCfElementFunctionEntry(ADD_ALLOWED_SENDER_ONCHAIN_INDEX);
+
+        _allowedSenders[sender] = true;
+        emit AddedAllowedSenderOnchain(sender);
+
+        _addCfElementFunctionExit(-ADD_ALLOWED_SENDER_ONCHAIN_INDEX, true);
+    }
+
     /**
      * Removes address so that they will not get served when calling the engine. Transaction from these addresses will get reverted.
      * @param senders list of address to stop service.
@@ -142,6 +169,10 @@ contract SphereXEngine is ISphereXEngine, AccessControlDefaultAdminRules {
         emit RemovedAllowedPatterns(patterns);
     }
 
+    function grantSenderAdderRole(address newSenderAdder) external onlyOperator {
+        _grantRole(SENDER_ADDER_ROLE, newSenderAdder);
+    }
+
     // ============ CF ============
 
     /**