Fix issues for security audit

contracts/Account.sol

@@ -12,16 +12,24 @@ contract AccountFactory is AccountFactoryBase {
     /// Logs the address of the created account on-chain
     event AccountCreated(address contractAddress);
 
-    /// @notice Function to create a clone
-    /// @param target address of master contract to clone
-    function clone(address target)
+    /// @notice This function creates a proxy of the target contract as per ERC-1167.
+    /// @param target address of target master contract to clone
+    function createProxy(address target)
         public virtual override {
+        // Note that this function creates a proxy contract to make contract deployment much cheaper. However, subsequent calls
+        // to the proxy contract are forwarded to the original contract instance making subsequent invocations slightly more expensive.
+        // As a result, consider the number of calls your application will make to the KMaaS account to determine whether creating
+        // a proxy contract or deploying the Account contract itself is gas-optimal.
         bytes20 targetBytes = bytes20(target);
         address contractAddr;
         assembly {
             let contractClone := mload(0x40)
+            // This contains 1. the init code to deploy the contract 2. bytes to copy calldata into memory and push the address
+            // of the target contract
             mstore(contractClone, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
+            // Store the address of the target contract
             mstore(add(contractClone, 0x14), targetBytes)
+            // This contains the code to execute delegatecall to the target address, as well as return/revert based on function execution
             mstore(add(contractClone, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)
             contractAddr := create(0, contractClone, 0x37)
         }
@@ -95,59 +103,6 @@ contract Account is AccountBase {
         return EthereumUtils.sign(publicKey, privateKey, digest);
     }
 
-    /// @notice Create a proxy transaction that the `proxy` function will then execute
-    /// This is helpful for using this account as a Gasless Relayer and code is taken from
-    /// https://github.com/oasisprotocol/sapphire-paratime/blob/main/examples/onchain-signer/contracts/Gasless.sol#L23
-    /// @param in_contract Address of contract to call
-    /// @param in_data Data to pass to the contract
-    /// @param nonce Nonce for the transaction
-    /// @returns Transaction encoded in EIP-155 format with signature
-    function makeProxyTx(
-        address in_contract,
-        bytes memory in_data,
-        uint64 nonce
-    ) external view authorized
-    returns (bytes memory output) {
-        bytes memory data = abi.encode(in_contract, in_data);
-
-        return
-        EIP155Signer.sign(
-            publicKey,
-            privateKey,
-            EIP155Signer.EthTx({
-                nonce: nonce,
-                gasPrice: 100_000_000_000,
-                gasLimit: 250000,
-                to: address(this),
-                value: 0,
-                data: abi.encodeCall(this.proxy, data),
-                chainId: block.chainid
-            })
-        );
-    }
-
-
-    /// @notice Function that is called by the makeProxyTx function once the signed transaction
-    /// is submitted
-    /// @param data Data passed in by makeProxyTx that is unpacked into (address, bytes)
-    function proxy(bytes memory data) external authorized payable {
-        (address addr, bytes memory subcallData) = abi.decode(
-            data,
-            (address, bytes)
-        );
-        (bool success, bytes memory outData) = addr.call{value: msg.value}(
-            subcallData
-        );
-        if (!success) {
-            // Add inner-transaction meaningful data in case of error.
-            assembly {
-                revert(add(outData, 32), mload(outData))
-            }
-        }
-
-        nonce += 1;
-    }
-
 
     /// @notice Call another contract. Useful if other contracts depend on authorization from this one
     /// This function is potentially state-changing and is payable as well

contracts/AccountBase.sol

@@ -6,7 +6,7 @@ import {EIP155Signer} from "@oasisprotocol/sapphire-contracts/contracts/EIP155Si
 
 // A contract to create per-identity account.
 abstract contract AccountFactoryBase {
-    function clone (address starterOwner)
+    function createProxy(address target)
     public virtual;
 }
 
@@ -31,7 +31,7 @@ abstract contract AccountBase {
     mapping (address => uint256) permission;
 
     /// @notice Function to check permissions
-    /// @param Address to check
+    /// @param grantee Address to check
     function hasPermission(address grantee) public virtual view
     returns (bool);
 

contracts/AccountWithSymKey.sol

@@ -45,7 +45,7 @@ contract AccountWithSymKey is Account {
     /// @notice Encrypt in_data with the named symmetric key.
     /// @param name Key name
     /// @param in_data Bytes to encrypt
-    /// @returns bytes array that contains both the nonce as well as encrypted output
+    /// @return out_data bytes array that contains both the nonce as well as encrypted output
     function encryptSymKey(string calldata name, bytes memory in_data)
     public virtual view authorized
     returns (bytes memory out_data) {
@@ -59,7 +59,7 @@ contract AccountWithSymKey is Account {
     /// @notice Decrypt in_data with the named symmetric key
     /// @param name Key name
     /// @param in_data Bytes to decrypt
-    /// @return Plaintext bytes
+    /// @return out_data Plaintext bytes
     function decryptSymKey(string calldata name, bytes memory in_data)
     public view authorized
     returns (bytes memory out_data) {

contracts/Validator.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.9;
 
 /// @title Contract to forward calls to an account once credentials are validated
 contract Validator {
+    error InvalidCredentials();
     type CredentialType is uint32;
 
     // Some examples defined. Can be extended to other types.
@@ -39,7 +40,9 @@ contract Validator {
     }
 
     modifier authorized(address account, bytes calldata credData) {
-        require(validate(account, credData), "Specified credentials invalid");
+        if (!validate(account, credData)) {
+            revert InvalidCredentials();
+        }
         _;
     }
 

hardhat.config.ts

@@ -1,7 +1,6 @@
 import '@oasisprotocol/sapphire-hardhat';
 import { HardhatUserConfig } from "hardhat/config";
 import "@nomicfoundation/hardhat-toolbox";
-import "./tasks/deploy";
 
 require('dotenv').config();
 

test/Basic.ts

@@ -16,13 +16,13 @@ describe('Basic test', () => {
         this.timeout(100000);
         [account1, account2] = await ethers.getSigners();
         const kmaasContractFactory = await ethers.getContractFactory('Account', account1);
-        const kmaasMasterContract = await kmaasContractFactory.deploy()
+        var kmaasMasterContract = await kmaasContractFactory.deploy()
         await kmaasMasterContract.waitForDeployment();
         const kmaasMasterContractAddr = await kmaasMasterContract.getAddress();
         const factory = await ethers.getContractFactory('AccountFactory', account1);
         const contract = await factory.deploy();
         await contract.waitForDeployment();
-        const kmaasTx = await contract.clone(kmaasMasterContractAddr);
+        const kmaasTx = await contract.createProxy(kmaasMasterContractAddr);
         const kmaasReceipt = await kmaasTx.wait();
         // Grab the KMaaS Account address from the events of the transaction
         const kmaasAddress = kmaasReceipt.logs[0].args[0];

test/SymKey.ts

@@ -21,7 +21,7 @@ describe("Symmetric Keys test", () => {
         const factoryContractFactory = await ethers.getContractFactory('AccountFactory', account);
         const factoryContract = await factoryContractFactory.deploy();
         await factoryContract.waitForDeployment();
-        const kmaasTx = await factoryContract.clone(masterContractAddr);
+        const kmaasTx = await factoryContract.createProxy(masterContractAddr);
         const kmaasReceipt = await kmaasTx.wait();
 
         // Grab the KMaaS Account address from the events of the transaction

test/Validator.ts

@@ -1,6 +1,7 @@
 import { ethers } from "hardhat";
 
 const { expect } = require("chai");
+import { ErrorDecoder } from 'ethers-decode-error';
 
 
 describe("Validator test", function () {
@@ -28,8 +29,9 @@ describe("Validator test", function () {
         };
 
         const validatorFactory = await ethers.getContractFactory('Validator', account1);
-        validator = await validatorFactory.deploy(await kmaasInstance.getAddress(), cred);
-        await validator.waitForDeployment();
+        const validatorTx = await validatorFactory.deploy();
+        const val = await validatorTx.waitForDeployment();
+        validator = val.connect(account1);
         var validatorAddress = await validator.getAddress();
         var addAccountTx = await validator.addAccount(kmaasAddress, cred);
         await addAccountTx.wait();
@@ -79,4 +81,5 @@ describe("Validator test", function () {
         var recoveredAddress = ethers.verifyMessage(text, digestSignatureObj)
         expect(recoveredAddress).to.equal(publicKey);
     });
+
 })