fix: Use OpenZeppelin Ownable and ReentrancyGuard for HCHelper

as suggested by audit. The index of the ResponseCache storage
slot is now detected rather
than being hard-coded.
 Changes to be committed:
	modified:   bin/rundler/src/cli/mod.rs
	modified:   crates/rpc/src/eth/api.rs
	modified:   crates/rpc/src/eth/router.rs
	modified:   crates/types/contracts/src/hc0_7/HCHelper.sol
	modified:   crates/types/src/hybrid_compute.rs

bin/rundler/src/cli/mod.rs

@@ -33,6 +33,7 @@ use rundler_sim::{
     EstimationSettings, PrecheckSettings, PriorityFeeMode, SimulationSettings, MIN_CALL_GAS_LIMIT,
 };
 use rundler_types::hybrid_compute;
+use rundler_types::contracts::v0_7::hc_helper::HCHelper;
 
 /// Main entry point for the CLI
 ///
@@ -53,17 +54,19 @@ pub async fn run() -> anyhow::Result<()> {
 
     let cs = chain_spec::resolve_chain_spec(&opt.common.network, &opt.common.chain_spec);
     tracing::info!("Chain spec: {:#?}", cs);
+    let node_http = opt.common.node_http.clone().expect("must provide node_http");
+    let p2 = rundler_provider::new_provider(&node_http, None)?;
+    let hx = HCHelper::new(opt.common.hc_helper_addr, p2);
+    let slot_idx = hx.response_slot().await.expect("Failed to get ResponseSlot");
 
     hybrid_compute::init(
         opt.common.hc_helper_addr,
         opt.common.hc_sys_account,
         opt.common.hc_sys_owner,
         opt.common.hc_sys_privkey,
         cs.clone(),
-        opt.common
-            .node_http
-            .clone()
-            .expect("Must provide node_http"),
+        node_http,
+        slot_idx,
     );
 
     match opt.command {

crates/rpc/src/eth/api.rs

@@ -24,7 +24,8 @@ use jsonrpsee::{
 };
 use rundler_types::{
     chain::ChainSpec,
-    contracts::v0_6::{hc_helper::HCHelper as HH2, simple_account::SimpleAccount},
+    contracts::v0_6::{simple_account::SimpleAccount},
+    contracts::v0_7::{hc_helper::HCHelper},
     hybrid_compute,
     pool::Pool,
     UserOperation, UserOperationOptionalGas, UserOperationVariant,
@@ -186,7 +187,7 @@ where
         );
         let p2 = rundler_provider::new_provider(&self.hc.node_http, None)?;
 
-        let hx = HH2::new(self.hc.helper_addr, p2.clone());
+        let hx = HCHelper::new(self.hc.helper_addr, p2.clone());
         let url = hx.registered_callers(ep_addr).await.expect("url_decode").1;
         println!("HC registered_caller url {:?}", url);
 

crates/rpc/src/eth/router.rs

@@ -198,7 +198,6 @@ impl EntryPointRouter {
             .get_nonce(addr, key)
             .await
             .map_err(Into::into)
-        //Ok(U256::from(0))
     }
 
     pub(crate) async fn check_signature(

crates/types/contracts/src/hc0_7/HCHelper.sol

@@ -3,23 +3,19 @@ pragma solidity ^0.8.12;
 
 import "account-abstraction/v0_7/interfaces/INonceManager.sol";
 import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
+import "@openzeppelin/contracts/access/Ownable.sol";
 
-contract HCHelper {
+contract HCHelper is ReentrancyGuard, Ownable {
     using SafeERC20 for IERC20;
 
-    event OwnershipTransferred (address previousOwner, address newOwner);
     event SystemAccountSet(address oldAccount, address newAccount);
     event RegisteredUrl(address contract_addr, string url);
     event TokenWithdrawal(address withdrawTo, uint256 amount);
 
     // Response data is stored here by PutResponse() and then consumed by TryCallOffchain().
-    // The storage slot must not be changed unless the corresponding code is updated in the Bundler.
-    // In particular, using an external implementation of Ownable would mess with the slot order.
     mapping(bytes32=>bytes)  ResponseCache;
 
-    // Owner (creator) of this contract.
-    address public owner;
-
     // BOBA token address
     address public tokenAddr;
 
@@ -44,22 +40,19 @@ contract HCHelper {
     address immutable entryPoint;
 
     // Constructor
-    constructor(address _entryPoint, address _tokenAddr, address _owner) {
+    constructor(address _entryPoint, address _tokenAddr, address _owner) Ownable(_owner) {
 	entryPoint = _entryPoint;
 	tokenAddr = _tokenAddr;
-        owner = _owner;
     }
 
     // Change the SystemAccount address (used for error responses)
-    function SetSystemAccount(address _systemAccount) public {
-        require(msg.sender == owner, "Only owner");
+    function SetSystemAccount(address _systemAccount) public onlyOwner {
         emit SystemAccountSet(systemAccount, _systemAccount);
         systemAccount = _systemAccount;
     }
 
     // Temporary method, until an auto-registration protocol is developed.
-    function RegisterUrl(address contract_addr, string calldata url) public {
-        require(msg.sender == owner, "Only owner");
+    function RegisterUrl(address contract_addr, string calldata url) public onlyOwner {
         require(bytes(url).length > 0, "URL cannot be empty");
         RegisteredCallers[contract_addr].owner = msg.sender;
         RegisteredCallers[contract_addr].url = url;
@@ -68,33 +61,24 @@ contract HCHelper {
 
     // Set or change the per-call token price (0 is allowed). Does not affect
     // existing credit balances, only applies to new AddCredit() calls.
-    function SetPrice(uint256 _pricePerCall) public {
-        require(msg.sender == owner, "Only owner");
+    function SetPrice(uint256 _pricePerCall) public onlyOwner {
 	pricePerCall = _pricePerCall;
     }
 
     // Purchase credits allowing the specified contract to perform HC calls.
     // The token cost is (pricePerCall() * numCredits) and is non-refundable
-    function AddCredit(address contract_addr, uint256 numCredits) public {
+    function AddCredit(address contract_addr, uint256 numCredits) public nonReentrant {
         uint256 tokenPrice = numCredits * pricePerCall;
         RegisteredCallers[contract_addr].credits += numCredits;
         IERC20(tokenAddr).safeTransferFrom(msg.sender, address(this), tokenPrice);
     }
 
     // Allow the owner to withdraw tokens
-    function WithdrawTokens(uint256 amount, address withdrawTo) public {
-        require(msg.sender == owner, "Only owner");
+    function WithdrawTokens(uint256 amount, address withdrawTo) public onlyOwner nonReentrant {
         emit TokenWithdrawal(withdrawTo, amount);
         IERC20(tokenAddr).safeTransfer(withdrawTo, amount);
     }
 
-    // Transer ownership
-    function transferOwnership(address newOwner) public {
-        require(msg.sender == owner, "Only owner");
-        emit OwnershipTransferred(owner, newOwner);
-        owner = newOwner;
-    }
-
     // Called from a HybridAccount contract, to populate the response which it will
     // subsequently request in TryCallOffchain()
     function PutResponse(bytes32 subKey, bytes calldata response) public {
@@ -205,4 +189,12 @@ contract HCHelper {
 	    }
 	}
     }
+
+    // Returns the slot index of ResponseCache, needed by the bundler.
+    // This index is affected by the OpenZeppelin libraries like Ownable.
+    function ResponseSlot() public pure returns (uint256 ret) {
+        assembly {
+            ret := ResponseCache.slot
+        }
+    }
 }

crates/types/src/hybrid_compute.rs

@@ -104,6 +104,8 @@ pub struct HcCfg {
     pub node_http: String,
     /// Temporary workaround
     pub from_addr: Address,
+    /// Index of ResponseCache slot in HCHelper
+    pub slot_idx: U256,
 }
 
 //pub static mut HC_CONFIG: HcCfg = HcCfg { helper_addr:Address::zero(), sys_account:Address::zero(),  sys_owner:Address::zero(), sys_privkey:H256::zero(), entry_point: Address::zero(), chain_id: 0, node_http:String::new(), from_addr: Address::zero()};
@@ -118,6 +120,7 @@ pub static HC_CONFIG: Lazy<Mutex<HcCfg>> = Lazy::new(|| {
         chain_spec: ChainSpec::default(),
         node_http: String::new(),
         from_addr: Address::zero(),
+        slot_idx: U256::from(0),
     };
     Mutex::new(c)
 });
@@ -130,6 +133,7 @@ pub fn init(
     sys_privkey: H256,
     chain_spec: ChainSpec,
     node_http: String,
+    slot_idx: U256,
 ) {
     let mut cfg = HC_CONFIG.lock().unwrap();
 
@@ -139,6 +143,7 @@ pub fn init(
     cfg.sys_privkey = sys_privkey;
     cfg.chain_spec = chain_spec;
     cfg.node_http.clone_from(&node_http);
+    cfg.slot_idx = slot_idx;
 }
 
 /// Set the EOA address which the bundler is using. Erigon, but not geth, needs this for tx simulation
@@ -210,11 +215,11 @@ pub fn hc_map_key(revert_data: &Bytes) -> H256 {
 
 /// Calculates the HCHelper storage slot key for a ResponseCache entry
 pub fn hc_storage_key(map_key: H256) -> H256 {
-    let slot_idx = "0x0000000000000000000000000000000000000000000000000000000000000000"
-        .parse::<Bytes>()
-        .unwrap();
+    let cfg = HC_CONFIG.lock().unwrap();
+    let slot_idx_bytes:Bytes = cfg.slot_idx.encode().into();
+
     let storage_key: H256 =
-        keccak256([Bytes::from(map_key.to_fixed_bytes()), slot_idx].concat()).into();
+        keccak256([Bytes::from(map_key.to_fixed_bytes()), slot_idx_bytes].concat()).into();
     storage_key
 }
 
@@ -704,6 +709,7 @@ mod test {
                 .unwrap(),
             ChainSpec::default(),
             "http://test.local/rpc".to_string(),
+            U256::from(2),
         );
         set_signer(
             "0x0000000000000000000000000000000000000005"
@@ -729,6 +735,7 @@ mod test {
             from_addr: "0x0000000000000000000000000000000000000005"
                 .parse::<Address>()
                 .unwrap(),
+            slot_idx: U256::from(2),
         };
         let cfg: HcCfg = HC_CONFIG.lock().unwrap().clone();
         // chain_spec doesn't support PartialEq
@@ -843,6 +850,7 @@ mod test {
             from_addr: "0x0000000000000000000000000000000000000005"
                 .parse::<Address>()
                 .unwrap(),
+            slot_idx: U256::from(0),
         };
 
         let wallet = LocalWallet::from_bytes(&cfg.sys_privkey.to_fixed_bytes()).unwrap();