From e102f13ebf646df140564098513d139b3681bf7b Mon Sep 17 00:00:00 2001 From: Rishi Balakrishnan Date: Mon, 14 Oct 2024 20:25:34 -0400 Subject: [PATCH 1/2] Fix issues for security audit: - remove makeProxyTx and proxy - update comments for AccountFactory - Add custom error for Validator invalid credentials --- contracts/Account.sol | 67 +++++++---------------------------------- contracts/Validator.sol | 5 ++- test/Basic.ts | 4 +-- test/SymKey.ts | 2 +- test/Validator.ts | 7 +++-- 5 files changed, 23 insertions(+), 62 deletions(-) diff --git a/contracts/Account.sol b/contracts/Account.sol index f297153..bbdb141 100644 --- a/contracts/Account.sol +++ b/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 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 diff --git a/contracts/Validator.sol b/contracts/Validator.sol index 9ea770f..0fbb75b 100644 --- a/contracts/Validator.sol +++ b/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(); + } _; } diff --git a/test/Basic.ts b/test/Basic.ts index b24e212..f4f0ec6 100644 --- a/test/Basic.ts +++ b/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]; diff --git a/test/SymKey.ts b/test/SymKey.ts index a6b19cd..6f9c3aa 100644 --- a/test/SymKey.ts +++ b/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 diff --git a/test/Validator.ts b/test/Validator.ts index cb9b450..2349a04 100644 --- a/test/Validator.ts +++ b/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); }); + }) \ No newline at end of file From 7a7eab11109291b48cdbddef493be3d130695260 Mon Sep 17 00:00:00 2001 From: Rishi Balakrishnan Date: Tue, 15 Oct 2024 03:29:04 -0400 Subject: [PATCH 2/2] Misc documentation / config fixes --- contracts/Account.sol | 2 +- contracts/AccountBase.sol | 4 ++-- contracts/AccountWithSymKey.sol | 4 ++-- hardhat.config.ts | 1 - 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/contracts/Account.sol b/contracts/Account.sol index bbdb141..c0447e3 100644 --- a/contracts/Account.sol +++ b/contracts/Account.sol @@ -13,7 +13,7 @@ contract AccountFactory is AccountFactoryBase { event AccountCreated(address contractAddress); /// @notice This function creates a proxy of the target contract as per ERC-1167. - /// @param address of target master contract to clone + /// @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 diff --git a/contracts/AccountBase.sol b/contracts/AccountBase.sol index eeba8cb..423b2e0 100644 --- a/contracts/AccountBase.sol +++ b/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); diff --git a/contracts/AccountWithSymKey.sol b/contracts/AccountWithSymKey.sol index 5702670..04aef23 100644 --- a/contracts/AccountWithSymKey.sol +++ b/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) { diff --git a/hardhat.config.ts b/hardhat.config.ts index 725d64f..e6edc68 100644 --- a/hardhat.config.ts +++ b/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();