From 82ac60192e533a9f2faefe75cb39d747be008833 Mon Sep 17 00:00:00 2001 From: Aaryamann Challani <43716372+rymnc@users.noreply.github.com> Date: Wed, 24 Jul 2024 12:46:38 +0530 Subject: [PATCH] chore(sc_keystore): add Ownable to the contracts for access control (#21) * forge install: openzeppelin-contracts v5.0.2 * chore(contracts): add ownable for access control on contract * fix: add test * fmt * fix: add user address instead of defaulting to msg.sender --- .gitmodules | 3 +++ contracts/lib/openzeppelin-contracts | 1 + contracts/remappings.txt | 1 + contracts/script/Deploy.s.sol | 8 ++++++-- contracts/src/IScKeystore.sol | 6 +++++- contracts/src/ScKeystore.sol | 17 ++++++++++------- contracts/test/ScKeystore.t.sol | 18 +++++++++++++++--- 7 files changed, 41 insertions(+), 13 deletions(-) create mode 160000 contracts/lib/openzeppelin-contracts diff --git a/.gitmodules b/.gitmodules index 3459f77..27735a3 100644 --- a/.gitmodules +++ b/.gitmodules @@ -2,3 +2,6 @@ branch = "v1" path = contracts/lib/forge-std url = https://github.com/foundry-rs/forge-std +[submodule "contracts/lib/openzeppelin-contracts"] + path = contracts/lib/openzeppelin-contracts + url = https://github.com/OpenZeppelin/openzeppelin-contracts diff --git a/contracts/lib/openzeppelin-contracts b/contracts/lib/openzeppelin-contracts new file mode 160000 index 0000000..dbb6104 --- /dev/null +++ b/contracts/lib/openzeppelin-contracts @@ -0,0 +1 @@ +Subproject commit dbb6104ce834628e473d2173bbc9d47f81a9eec3 diff --git a/contracts/remappings.txt b/contracts/remappings.txt index feaba2d..b46e1bf 100644 --- a/contracts/remappings.txt +++ b/contracts/remappings.txt @@ -1 +1,2 @@ forge-std/=lib/forge-std/src/ +Openzeppelin/=lib/openzeppelin-contracts/contracts diff --git a/contracts/script/Deploy.s.sol b/contracts/script/Deploy.s.sol index a0b853e..f04263c 100644 --- a/contracts/script/Deploy.s.sol +++ b/contracts/script/Deploy.s.sol @@ -6,8 +6,12 @@ import { BaseScript } from "./Base.s.sol"; import { DeploymentConfig } from "./DeploymentConfig.s.sol"; contract Deploy is BaseScript { - function run() public broadcast returns (ScKeystore scKeystore, DeploymentConfig deploymentConfig) { + function run(address initialOwner) + public + broadcast + returns (ScKeystore scKeystore, DeploymentConfig deploymentConfig) + { deploymentConfig = new DeploymentConfig(broadcaster); - scKeystore = new ScKeystore(); + scKeystore = new ScKeystore(initialOwner); } } diff --git a/contracts/src/IScKeystore.sol b/contracts/src/IScKeystore.sol index ccc1449..5e2e4f0 100644 --- a/contracts/src/IScKeystore.sol +++ b/contracts/src/IScKeystore.sol @@ -13,8 +13,12 @@ struct UserInfo { interface IScKeystore { function userExists(address user) external view returns (bool); - function addUser(bytes calldata signaturePubKey, KeyPackage calldata keyPackage) external; + + function addUser(address user, bytes calldata signaturePubKey, KeyPackage calldata keyPackage) external; + function getUser(address user) external view returns (UserInfo memory); + function addKeyPackage(KeyPackage calldata) external; + function getAvailableKeyPackage(address user) external view returns (KeyPackage memory); } diff --git a/contracts/src/ScKeystore.sol b/contracts/src/ScKeystore.sol index bea1119..5f451c2 100644 --- a/contracts/src/ScKeystore.sol +++ b/contracts/src/ScKeystore.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.24; +import { Ownable } from "Openzeppelin/access/Ownable.sol"; import { IScKeystore, UserInfo, KeyPackage } from "./IScKeystore.sol"; error UserAlreadyExists(); @@ -8,30 +9,32 @@ error MalformedKeyPackage(); error MalformedUserInfo(); error UserDoesNotExist(); -contract ScKeystore is IScKeystore { +contract ScKeystore is Ownable, IScKeystore { event UserAdded(address user, bytes signaturePubKey); event UserKeyPackageAdded(address indexed user, uint256 index); mapping(address user => UserInfo userInfo) private users; KeyPackage[] private keyPackages; + constructor(address initialOwner) Ownable(initialOwner) { } + function userExists(address user) public view returns (bool) { return users[user].signaturePubKey.length > 0; } - function addUser(bytes calldata signaturePubKey, KeyPackage calldata keyPackage) external { + function addUser(address user, bytes calldata signaturePubKey, KeyPackage calldata keyPackage) external onlyOwner { if (signaturePubKey.length == 0) revert MalformedUserInfo(); if (keyPackage.data.length == 0) revert MalformedKeyPackage(); - if (userExists(msg.sender)) revert UserAlreadyExists(); + if (userExists(user)) revert UserAlreadyExists(); keyPackages.push(keyPackage); uint256 keyPackageIndex = keyPackages.length - 1; - users[msg.sender] = UserInfo(new uint256[](0), signaturePubKey); - users[msg.sender].signaturePubKey = signaturePubKey; - users[msg.sender].keyPackageIndices.push(keyPackageIndex); + users[user] = UserInfo(new uint256[](0), signaturePubKey); + users[user].signaturePubKey = signaturePubKey; + users[user].keyPackageIndices.push(keyPackageIndex); - emit UserAdded(msg.sender, signaturePubKey); + emit UserAdded(user, signaturePubKey); } function getUser(address user) external view returns (UserInfo memory) { diff --git a/contracts/test/ScKeystore.t.sol b/contracts/test/ScKeystore.t.sol index b8a44b6..d79c57b 100644 --- a/contracts/test/ScKeystore.t.sol +++ b/contracts/test/ScKeystore.t.sol @@ -4,6 +4,7 @@ pragma solidity >=0.8.19 <0.9.0; import { Test } from "forge-std/Test.sol"; import { Deploy } from "../script/Deploy.s.sol"; import { DeploymentConfig } from "../script/DeploymentConfig.s.sol"; +import "forge-std/console.sol"; import "../src/ScKeystore.sol"; // solhint-disable-line contract ScKeystoreTest is Test { @@ -13,12 +14,16 @@ contract ScKeystoreTest is Test { function setUp() public virtual { Deploy deployment = new Deploy(); - (s, deploymentConfig) = deployment.run(); + (s, deploymentConfig) = deployment.run(address(this)); } function addUser() internal { KeyPackage memory keyPackage = KeyPackage({ data: new bytes[](1) }); - s.addUser("0x", keyPackage); + s.addUser(address(this), "0x", keyPackage); + } + + function test__owner() public view { + assert(s.owner() == address(this)); } function test__userExists__returnsFalse__whenUserDoesNotExist() public view { @@ -27,7 +32,7 @@ contract ScKeystoreTest is Test { function test__addUser__reverts__whenUserInfoIsMalformed() public { vm.expectRevert(MalformedUserInfo.selector); - s.addUser("", KeyPackage({ data: new bytes[](0) })); + s.addUser(address(this), "", KeyPackage({ data: new bytes[](0) })); } function test__addUser__reverts__whenUserAlreadyExists() public { @@ -41,6 +46,13 @@ contract ScKeystoreTest is Test { assert(s.userExists(address(this))); } + function test__addUser__reverts__whenSenderIsNotOwner() public { + vm.prank(address(0)); + vm.expectRevert(); + addUser(); + vm.stopPrank(); + } + function test__getUser__returnsUserInfo__whenUserExists() public { addUser(); UserInfo memory userInfo = s.getUser(address(this));