From 2ba1e498b12247b10252b3fd4e90a4387fbe6a3d Mon Sep 17 00:00:00 2001 From: Jacob Caban-Tomski Date: Fri, 28 May 2021 09:07:11 -0600 Subject: [PATCH] Add access control to setRollupAddress in DepositManager and Vault contracts. Create ImmutableOwnable as a slimmed down version of OpenZeppelin's Ownable. Inherit OpenZeppelin's Initalizable and ImmutableOwnable in DepositManger and Vault contracts. Add initializer and onlyOwner modifiers to setRollupAddress to limit who and how can set those contract's Rollup address. Modify deploy to wait until Vault's Rollup address set is mined. Add randomAddress utility function to aid in testing. --- contracts/DepositManager.sol | 21 +++++++++--- contracts/Vault.sol | 17 ++++++---- contracts/libs/ImmutableOwnable.sol | 34 ++++++++++++++++++++ test/fast/depositManager.test.ts | 50 +++++++++++++++++++++++++++++ test/fast/rollup.test.ts | 7 +--- test/fast/vault.test.ts | 43 +++++++++++++++++++++++++ ts/deploy.ts | 2 +- ts/utils.ts | 17 +++++++--- 8 files changed, 170 insertions(+), 21 deletions(-) create mode 100644 contracts/libs/ImmutableOwnable.sol create mode 100644 test/fast/depositManager.test.ts create mode 100644 test/fast/vault.test.ts diff --git a/contracts/DepositManager.sol b/contracts/DepositManager.sol index 06100df8..d7556e47 100644 --- a/contracts/DepositManager.sol +++ b/contracts/DepositManager.sol @@ -1,11 +1,14 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.6.12; pragma experimental ABIEncoderV2; -import { Types } from "./libs/Types.sol"; -import { ITokenRegistry } from "./TokenRegistry.sol"; + import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/SafeERC20.sol"; +import { Initializable } from "@openzeppelin/contracts/proxy/Initializable.sol"; +import { Types } from "./libs/Types.sol"; +import { ImmutableOwnable } from "./libs/ImmutableOwnable.sol"; import { Rollup } from "./rollup/Rollup.sol"; +import { ITokenRegistry } from "./TokenRegistry.sol"; interface IDepositManager { event DepositQueued(uint256 pubkeyID, uint256 tokenID, uint256 l2Amount); @@ -90,7 +93,12 @@ contract DepositCore is SubtreeQueue { } } -contract DepositManager is DepositCore, IDepositManager { +contract DepositManager is + DepositCore, + IDepositManager, + Initializable, + ImmutableOwnable +{ using Types for Types.UserState; using SafeERC20 for IERC20; @@ -119,7 +127,12 @@ contract DepositManager is DepositCore, IDepositManager { vault = _vault; } - function setRollupAddress(address _rollup) public { + /** + * @notice Sets Rollup contract address. Can only be called once by owner + * @dev We assume DepositManager is deployed before Rollup + * @param _rollup Rollup contract address + */ + function setRollupAddress(address _rollup) external initializer onlyOwner { rollup = _rollup; } diff --git a/contracts/Vault.sol b/contracts/Vault.sol index c8a09051..32971bcf 100644 --- a/contracts/Vault.sol +++ b/contracts/Vault.sol @@ -1,15 +1,18 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.6.12; pragma experimental ABIEncoderV2; -import { Bitmap } from "./libs/Bitmap.sol"; + import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; -import { Rollup } from "./rollup/Rollup.sol"; -import { ITokenRegistry } from "./TokenRegistry.sol"; +import { Initializable } from "@openzeppelin/contracts/proxy/Initializable.sol"; +import { Bitmap } from "./libs/Bitmap.sol"; import { Types } from "./libs/Types.sol"; import { MerkleTree } from "./libs/MerkleTree.sol"; +import { ImmutableOwnable } from "./libs/ImmutableOwnable.sol"; +import { Rollup } from "./rollup/Rollup.sol"; +import { ITokenRegistry } from "./TokenRegistry.sol"; import { SpokeRegistry } from "./SpokeRegistry.sol"; -contract Vault { +contract Vault is Initializable, ImmutableOwnable { using Types for Types.MassMigrationCommitment; using Types for Types.Batch; @@ -26,9 +29,11 @@ contract Vault { } /** - @dev We assume Vault is deployed before Rollup + * @notice Sets Rollup contract address. Can only be called once by owner + * @dev We assume Vault is deployed before Rollup + * @param _rollup Rollup contract address */ - function setRollupAddress(Rollup _rollup) external { + function setRollupAddress(Rollup _rollup) external initializer onlyOwner { rollup = _rollup; } diff --git a/contracts/libs/ImmutableOwnable.sol b/contracts/libs/ImmutableOwnable.sol new file mode 100644 index 00000000..3b905726 --- /dev/null +++ b/contracts/libs/ImmutableOwnable.sol @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.6.12; + +/** + * Immutable, non-transferable version of openzeppelin/contracts/access/Ownable + */ +abstract contract ImmutableOwnable { + address private immutable _owner; + + /** + * @dev Initializes the contract setting the deployer as the initial owner. + */ + constructor() internal { + _owner = msg.sender; + } + + /** + * @dev Returns the address of the current owner. + */ + function owner() public view virtual returns (address) { + return _owner; + } + + /** + * @dev Throws if called by any account other than the owner. + */ + modifier onlyOwner() { + require( + msg.sender == _owner, + "ImmutableOwnable: caller is not the owner" + ); + _; + } +} diff --git a/test/fast/depositManager.test.ts b/test/fast/depositManager.test.ts new file mode 100644 index 00000000..f144a262 --- /dev/null +++ b/test/fast/depositManager.test.ts @@ -0,0 +1,50 @@ +import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; +import chai, { assert } from "chai"; +import chaiAsPromised from "chai-as-promised"; +import { ethers } from "hardhat"; +import { TESTING_PARAMS as params } from "../../ts/constants"; +import { randomAddress } from "../../ts/utils"; +import { DepositManagerFactory } from "../../types/ethers-contracts"; +import { DepositManager } from "../../types/ethers-contracts/DepositManager"; + +chai.use(chaiAsPromised); + +describe("DepositManager", () => { + let owner: SignerWithAddress; + let otherSigner: SignerWithAddress; + let depositManager: DepositManager; + + beforeEach(async function() { + const signers = await ethers.getSigners(); + owner = signers[0]; + otherSigner = signers[1]; + + const fakeAddress = randomAddress(); + depositManager = await new DepositManagerFactory(owner).deploy( + fakeAddress, + fakeAddress, + params.MAX_DEPOSIT_SUBTREE_DEPTH + ); + }); + + describe("setRollupAddress", () => { + it("fails if sender is not owner", async function() { + await assert.isRejected( + depositManager + .connect(otherSigner) + .setRollupAddress(randomAddress()), + /.*Ownable: caller is not the owner/ + ); + }); + + it("sets rollup contract address and fails if called again", async function() { + const rollupAddress = randomAddress(); + await depositManager.setRollupAddress(rollupAddress); + assert.equal(await depositManager.rollup(), rollupAddress); + await assert.isRejected( + depositManager.setRollupAddress(randomAddress()), + /.*Initializable: contract is already initialized/ + ); + }); + }); +}); diff --git a/test/fast/rollup.test.ts b/test/fast/rollup.test.ts index ac2210bc..df23da56 100644 --- a/test/fast/rollup.test.ts +++ b/test/fast/rollup.test.ts @@ -1,20 +1,15 @@ import { assert } from "chai"; -import crypto from "crypto"; import { ethers } from "hardhat"; import { TESTING_PARAMS as parameters } from "../../ts/constants"; import { generateDomainSeparatorFromRollup } from "../../ts/domain"; import { StateTree } from "../../ts/stateTree"; +import { randomAddress } from "../../ts/utils"; import { DepositManagerFactory, RollupFactory } from "../../types/ethers-contracts"; import { Rollup } from "../../types/ethers-contracts/Rollup"; -const randomAddress = () => { - const hex = crypto.randomBytes(20).toString("hex"); - return `0x${hex}`; -}; - describe("Rollup", () => { let rollup: Rollup; diff --git a/test/fast/vault.test.ts b/test/fast/vault.test.ts new file mode 100644 index 00000000..f0a24327 --- /dev/null +++ b/test/fast/vault.test.ts @@ -0,0 +1,43 @@ +import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; +import chai, { assert } from "chai"; +import chaiAsPromised from "chai-as-promised"; +import { ethers } from "hardhat"; +import { randomAddress } from "../../ts/utils"; +import { Vault } from "../../types/ethers-contracts/Vault"; +import { VaultFactory } from "../../types/ethers-contracts/VaultFactory"; + +chai.use(chaiAsPromised); + +describe("Vault", () => { + let owner: SignerWithAddress; + let otherSigner: SignerWithAddress; + let vault: Vault; + + beforeEach(async function() { + const signers = await ethers.getSigners(); + owner = signers[0]; + otherSigner = signers[1]; + + const fakeAddress = randomAddress(); + vault = await new VaultFactory(owner).deploy(fakeAddress, fakeAddress); + }); + + describe("setRollupAddress", () => { + it("fails if sender is not owner", async function() { + await assert.isRejected( + vault.connect(otherSigner).setRollupAddress(randomAddress()), + /.*Ownable: caller is not the owner/ + ); + }); + + it("sets rollup contract address and fails if called again", async function() { + const rollupAddress = randomAddress(); + await vault.setRollupAddress(rollupAddress); + assert.equal(await vault.rollup(), rollupAddress); + await assert.isRejected( + vault.setRollupAddress(randomAddress()), + /.*Initializable: contract is already initialized/ + ); + }); + }); +}); diff --git a/ts/deploy.ts b/ts/deploy.ts index 4820b3be..0f616b8c 100644 --- a/ts/deploy.ts +++ b/ts/deploy.ts @@ -138,7 +138,7 @@ export async function deployAll( ); await waitAndRegister(rollup, "rollup", verbose); - await vault.setRollupAddress(rollup.address); + await waitUntilMined(vault.setRollupAddress(rollup.address)); await waitUntilMined(depositManager.setRollupAddress(rollup.address)); const withdrawManager = await new WithdrawManagerFactory(signer).deploy( diff --git a/ts/utils.ts b/ts/utils.ts index 668f9c5d..76a855ed 100644 --- a/ts/utils.ts +++ b/ts/utils.ts @@ -1,15 +1,14 @@ -import { ethers } from "ethers"; -import { BigNumber } from "ethers"; +import { ethers, BigNumber, ContractTransaction } from "ethers"; import { randomBytes, hexlify, hexZeroPad, parseEther, BytesLike, - solidityKeccak256 + solidityKeccak256, + getAddress } from "ethers/lib/utils"; import { Wei } from "./interfaces"; -import { ContractTransaction } from "ethers"; import { assert, expect } from "chai"; import { Rollup } from "../types/ethers-contracts/Rollup"; @@ -65,6 +64,16 @@ export function randomLeaves(num: number): string[] { return leaves; } +/** + * Generates a random address. Usefully for testing when + * you don't need a valid address or contract. + * + * @returns Randomly generated address. + */ +export function randomAddress(): string { + return getAddress(randHex(20)); +} + // Simulate the tree depth of calling contracts/libs/MerkleTree.sol::MerkleTree.merklize // Make the depth as shallow as possible // the length 1 is a special case that the formula doesn't work