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