Merge pull request #597 from thehubbleproject/deposit-and-vault-acces…

…s-control

Add access control to setRollupAddress in DepositManager and Vault contracts

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;
     }
 

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;
     }
 

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"
+        );
+        _;
+    }
+}

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/
+            );
+        });
+    });
+});

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;
 

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/
+            );
+        });
+    });
+});

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(

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