Merge hacken audit fixes (#987)

* Prevent replayable messages on new `Inbox` versions (as enrolled on `AbacusConnectionManager`) (#977)

* Check return values of set interaction in ACM (#983)

* Add zero address validation (#984)

* Clarify default visibility (#985)

* Remove abicoder v2 annotation (#986)

* Fix connection client tests

* Fix router tests

* Fix sdk tests

solidity/app/test/abacusConnectionClient.test.ts

@@ -8,6 +8,8 @@ import {
   InterchainGasPaymaster,
   InterchainGasPaymaster__factory,
   Outbox__factory,
+  TestInbox__factory,
+  TestMultisigValidatorManager__factory,
 } from '@abacus-network/core';
 
 import {
@@ -68,7 +70,8 @@ describe('AbacusConnectionClient', async () => {
   });
 
   it('returns outbox from connection manager', async () => {
-    const outbox = nonOwner.address;
+    // must be contract
+    const outbox = connectionManager.address;
     expect(await connectionClient.outbox()).to.equal(
       ethers.constants.AddressZero,
     );
@@ -77,8 +80,14 @@ describe('AbacusConnectionClient', async () => {
   });
 
   it('returns inbox from connection manager', async () => {
-    const inbox = nonOwner.address;
     const domain = 1;
+    const remoteDomain = 2;
+    const validatorManager = await new TestMultisigValidatorManager__factory(
+      signer,
+    ).deploy(domain, [nonOwner.address], 1);
+    const inboxContract = await new TestInbox__factory(signer).deploy(domain);
+    await inboxContract.initialize(remoteDomain, validatorManager.address);
+    const inbox = inboxContract.address;
     expect(await connectionClient.isInbox(inbox)).to.equal(false);
     await connectionManager.enrollInbox(domain, inbox);
     expect(await connectionClient.isInbox(inbox)).to.equal(true);

solidity/app/test/router.test.ts

@@ -11,6 +11,9 @@ import {
   InterchainGasPaymaster__factory,
   Outbox,
   Outbox__factory,
+  TestInbox,
+  TestInbox__factory,
+  TestMultisigValidatorManager__factory,
 } from '@abacus-network/core';
 import { utils } from '@abacus-network/utils';
 
@@ -81,16 +84,24 @@ describe('Router', async () => {
   });
 
   describe('when initialized', () => {
+    let inbox: TestInbox;
+
     beforeEach(async () => {
       await router.initialize(connectionManager.address);
+      const validatorManger = await new TestMultisigValidatorManager__factory(
+        signer,
+      ).deploy(origin, [signer.address], 1);
+      inbox = await new TestInbox__factory(signer).deploy(destination);
+      await inbox.initialize(origin, validatorManger.address);
     });
 
     it('accepts message from enrolled inbox and router', async () => {
-      await connectionManager.enrollInbox(origin, signer.address);
+      await connectionManager.enrollInbox(origin, inbox.address);
       const remote = utils.addressToBytes32(nonOwner.address);
       await router.enrollRemoteRouter(origin, remote);
+      const recipient = utils.addressToBytes32(router.address);
       // Does not revert.
-      await router.handle(origin, remote, message);
+      await inbox.testHandle(origin, remote, recipient, message);
     });
 
     it('rejects message from unenrolled inbox', async () => {
@@ -104,13 +115,11 @@ describe('Router', async () => {
     });
 
     it('rejects message from unenrolled router', async () => {
-      await connectionManager.enrollInbox(origin, signer.address);
+      await connectionManager.enrollInbox(origin, inbox.address);
+      const recipient = utils.addressToBytes32(router.address);
+      const sender = utils.addressToBytes32(nonOwner.address);
       await expect(
-        router.handle(
-          origin,
-          utils.addressToBytes32(nonOwner.address),
-          message,
-        ),
+        inbox.testHandle(origin, sender, recipient, message),
       ).to.be.revertedWith('!router');
     });
 

solidity/core/contracts/AbacusConnectionManager.sol

@@ -1,13 +1,15 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.8.0;
-pragma abicoder v2;
 
 // ============ Internal Imports ============
 import {IOutbox} from "../interfaces/IOutbox.sol";
+import {IInbox} from "../interfaces/IInbox.sol";
 import {IAbacusConnectionManager} from "../interfaces/IAbacusConnectionManager.sol";
+import {IMultisigValidatorManager} from "../interfaces/IMultisigValidatorManager.sol";
 
 // ============ External Imports ============
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
+import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 
 /**
@@ -18,6 +20,7 @@ import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet
  */
 contract AbacusConnectionManager is IAbacusConnectionManager, Ownable {
     using EnumerableSet for EnumerableSet.AddressSet;
+    using EnumerableSet for EnumerableSet.Bytes32Set;
 
     // ============ Public Storage ============
 
@@ -26,7 +29,9 @@ contract AbacusConnectionManager is IAbacusConnectionManager, Ownable {
     // local Inbox address => remote Outbox domain
     mapping(address => uint32) public inboxToDomain;
     // remote Outbox domain => local Inbox addresses
-    mapping(uint32 => EnumerableSet.AddressSet) domainToInboxes;
+    mapping(uint32 => EnumerableSet.AddressSet) internal domainToInboxes;
+    // complete history of enrolled inbox domain hashes, even if unenrolled
+    EnumerableSet.Bytes32Set domainHashes;
 
     // ============ Events ============
 
@@ -62,6 +67,7 @@ contract AbacusConnectionManager is IAbacusConnectionManager, Ownable {
      * @param _outbox The address of the new local Outbox contract.
      */
     function setOutbox(address _outbox) external onlyOwner {
+        require(Address.isContract(_outbox), "outbox !contract");
         outbox = IOutbox(_outbox);
         emit OutboxSet(_outbox);
     }
@@ -72,10 +78,20 @@ contract AbacusConnectionManager is IAbacusConnectionManager, Ownable {
      * @param _inbox the address of the Inbox
      */
     function enrollInbox(uint32 _domain, address _inbox) external onlyOwner {
+        require(Address.isContract(_inbox), "inbox !contract");
         require(!isInbox(_inbox), "already inbox");
+
+        // prevent enrolling an inbox that matches any previously enrolled domain hash
+        bytes32 domainHash = getDomainHash(_inbox);
+        require(
+            !domainHashes.contains(domainHash),
+            "domain hash previously enrolled"
+        );
+        domainHashes.add(domainHash);
+
         // add inbox and domain to two-way mapping
         inboxToDomain[_inbox] = _domain;
-        domainToInboxes[_domain].add(_inbox);
+        require(domainToInboxes[_domain].add(_inbox), "already enrolled");
         emit InboxEnrolled(_domain, _inbox);
     }
 
@@ -126,6 +142,17 @@ contract AbacusConnectionManager is IAbacusConnectionManager, Ownable {
         return inboxToDomain[_inbox] != 0;
     }
 
+    /**
+     * @notice Check whether _inbox is enrolled
+     * @param _inbox the inbox to check for enrollment
+     * @return TRUE iff _inbox is enrolled
+     */
+    function getDomainHash(address _inbox) public view returns (bytes32) {
+        IInbox inbox = IInbox(_inbox);
+        address vm = inbox.validatorManager();
+        return IMultisigValidatorManager(vm).domainHash();
+    }
+
     // ============ Internal Functions ============
 
     /**
@@ -134,7 +161,7 @@ contract AbacusConnectionManager is IAbacusConnectionManager, Ownable {
      */
     function _unenrollInbox(address _inbox) internal {
         uint32 _currentDomain = inboxToDomain[_inbox];
-        domainToInboxes[_currentDomain].remove(_inbox);
+        require(domainToInboxes[_currentDomain].remove(_inbox), "not enrolled");
         inboxToDomain[_inbox] = 0;
         emit InboxUnenrolled(_currentDomain, _inbox);
     }

solidity/core/contracts/Inbox.sol

@@ -2,7 +2,7 @@
 pragma solidity >=0.8.0;
 
 // ============ Internal Imports ============
-import {Version0} from "./Version0.sol";
+import {Versioned} from "./upgrade/Versioned.sol";
 import {Mailbox} from "./Mailbox.sol";
 import {MerkleLib} from "./libs/Merkle.sol";
 import {Message} from "./libs/Message.sol";
@@ -19,7 +19,7 @@ import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/se
  * @notice Track root updates on Outbox, prove and dispatch messages to end
  * recipients.
  */
-contract Inbox is IInbox, ReentrancyGuardUpgradeable, Version0, Mailbox {
+contract Inbox is IInbox, ReentrancyGuardUpgradeable, Versioned, Mailbox {
     // ============ Libraries ============
 
     using MerkleLib for MerkleLib.Tree;

solidity/core/contracts/Outbox.sol

@@ -2,7 +2,7 @@
 pragma solidity >=0.8.0;
 
 // ============ Internal Imports ============
-import {Version0} from "./Version0.sol";
+import {Versioned} from "./upgrade/Versioned.sol";
 import {Mailbox} from "./Mailbox.sol";
 import {MerkleLib} from "./libs/Merkle.sol";
 import {Message} from "./libs/Message.sol";
@@ -20,7 +20,7 @@ import {IOutbox} from "../interfaces/IOutbox.sol";
  * Accepts submissions of fraudulent signatures
  * by the Validator and slashes the Validator in this case.
  */
-contract Outbox is IOutbox, Version0, MerkleTreeManager, Mailbox {
+contract Outbox is IOutbox, Versioned, MerkleTreeManager, Mailbox {
     // ============ Libraries ============
 
     using MerkleLib for MerkleLib.Tree;

solidity/core/contracts/test/TestInbox.sol

@@ -5,6 +5,7 @@ import "../Inbox.sol";
 
 contract TestInbox is Inbox {
     using Message for bytes32;
+    using TypeCasts for bytes32;
 
     constructor(uint32 _localDomain) Inbox(_localDomain) {} // solhint-disable-line no-empty-blocks
 
@@ -21,6 +22,19 @@ contract TestInbox is Inbox {
         _process(_message, _messageHash);
     }
 
+    function testHandle(
+        uint32 origin,
+        bytes32 sender,
+        bytes32 recipient,
+        bytes calldata body
+    ) external {
+        IMessageRecipient(recipient.bytes32ToAddress()).handle(
+            origin,
+            sender,
+            body
+        );
+    }
+
     function setMessageStatus(bytes32 _leaf, MessageStatus status) external {
         messages[_leaf] = status;
     }

solidity/core/contracts/test/TestMultisigValidatorManager.sol

@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.8.0;
-pragma abicoder v2;
 
 import {MultisigValidatorManager} from "../validator-manager/MultisigValidatorManager.sol";
 

solidity/core/contracts/upgrade/Versioned.sol

@@ -2,9 +2,9 @@
 pragma solidity >=0.6.11;
 
 /**
- * @title Version0
+ * @title Versioned
  * @notice Version getter for contracts
  **/
-contract Version0 {
+contract Versioned {
     uint8 public constant VERSION = 0;
 }

solidity/core/contracts/validator-manager/InboxValidatorManager.sol

@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.8.0;
-pragma abicoder v2;
 
 // ============ Internal Imports ============
 import {IInbox} from "../../interfaces/IInbox.sol";

solidity/core/contracts/validator-manager/MultisigValidatorManager.sol

@@ -1,18 +1,26 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.8.0;
-pragma abicoder v2;
+
+import {Versioned} from "../upgrade/Versioned.sol";
 
 // ============ External Imports ============
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
 import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 
+// ============ Internal Imports ============
+import {IMultisigValidatorManager} from "../../interfaces/IMultisigValidatorManager.sol";
+
 /**
  * @title MultisigValidatorManager
  * @notice Manages an ownable set of validators that ECDSA sign checkpoints to
  * reach a quorum.
  */
-abstract contract MultisigValidatorManager is Ownable {
+abstract contract MultisigValidatorManager is
+    IMultisigValidatorManager,
+    Ownable,
+    Versioned
+{
     // ============ Libraries ============
 
     using EnumerableSet for EnumerableSet.AddressSet;
@@ -218,6 +226,7 @@ abstract contract MultisigValidatorManager is Ownable {
      * @param _validator The validator to add to the validator set.
      */
     function _enrollValidator(address _validator) internal {
+        require(_validator != address(0), "zero address");
         require(validatorSet.add(_validator), "already enrolled");
         emit ValidatorEnrolled(_validator, validatorCount());
     }
@@ -247,10 +256,16 @@ abstract contract MultisigValidatorManager is Ownable {
     }
 
     /**
-     * @notice Hash of `_domain` concatenated with "ABACUS".
+     * @notice Hash of `_domain` concatenated with "ABACUS" and deployment version.
+     * @dev Domain hash is salted with deployment version to prevent validator signature replay.
      * @param _domain The domain to hash.
      */
     function _domainHash(uint32 _domain) internal pure returns (bytes32) {
-        return keccak256(abi.encodePacked(_domain, "ABACUS"));
+        if (VERSION > 0) {
+            return keccak256(abi.encodePacked(_domain, "ABACUS", VERSION));
+        } else {
+            // for backwards compatibility with initial deployment (VERSION == 0)
+            return keccak256(abi.encodePacked(_domain, "ABACUS"));
+        }
     }
 }

solidity/core/contracts/validator-manager/OutboxValidatorManager.sol

@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.8.0;
-pragma abicoder v2;
 
 // ============ Internal Imports ============
 import {IOutbox} from "../../interfaces/IOutbox.sol";

solidity/core/interfaces/IMailbox.sol

@@ -3,4 +3,6 @@ pragma solidity >=0.6.11;
 
 interface IMailbox {
     function localDomain() external view returns (uint32);
+
+    function validatorManager() external view returns (address);
 }

solidity/core/interfaces/IMultisigValidatorManager.sol

@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: MIT OR Apache-2.0
+pragma solidity >=0.6.0;
+
+interface IMultisigValidatorManager {
+    function domain() external view returns (uint32);
+
+    // The domain hash of the validator set's outbox chain.
+    function domainHash() external view returns (bytes32);
+
+    function threshold() external view returns (uint256);
+}

solidity/core/test/abacusConnectionManager.test.ts

@@ -5,10 +5,12 @@ import { ethers } from 'hardhat';
 import {
   AbacusConnectionManager,
   AbacusConnectionManager__factory,
+  MultisigValidatorManager,
   Outbox,
   Outbox__factory,
   TestInbox,
   TestInbox__factory,
+  TestMultisigValidatorManager__factory,
 } from '../types';
 
 const ONLY_OWNER_REVERT_MSG = 'Ownable: caller is not the owner';
@@ -17,6 +19,7 @@ const remoteDomain = 2000;
 
 describe('AbacusConnectionManager', async () => {
   let connectionManager: AbacusConnectionManager,
+    validatorManager: MultisigValidatorManager,
     enrolledInbox: TestInbox,
     signer: SignerWithAddress,
     nonOwner: SignerWithAddress;
@@ -31,9 +34,16 @@ describe('AbacusConnectionManager', async () => {
 
     const inboxFactory = new TestInbox__factory(signer);
     enrolledInbox = await inboxFactory.deploy(localDomain);
-    // The ValidatorManager is unused in these tests *but* needs to be a
-    // contract.
-    await enrolledInbox.initialize(remoteDomain, outbox.address);
+
+    const validatorManagerFactory = new TestMultisigValidatorManager__factory(
+      signer,
+    );
+    validatorManager = await validatorManagerFactory.deploy(
+      remoteDomain,
+      [signer.address],
+      1,
+    );
+    await enrolledInbox.initialize(remoteDomain, validatorManager.address);
 
     const connectionManagerFactory = new AbacusConnectionManager__factory(
       signer,
@@ -80,10 +90,19 @@ describe('AbacusConnectionManager', async () => {
       .false;
   });
 
-  it('Owner can enroll a inbox', async () => {
+  it('Owner can enroll multiple inboxes with different domain hashes', async () => {
     const newRemoteDomain = 3000;
     const inboxFactory = new TestInbox__factory(signer);
     const newInbox = await inboxFactory.deploy(localDomain);
+    const validatorManagerFactory = new TestMultisigValidatorManager__factory(
+      signer,
+    );
+    const newValidatorManager = await validatorManagerFactory.deploy(
+      newRemoteDomain,
+      [signer.address],
+      1,
+    );
+    await newInbox.initialize(newRemoteDomain, newValidatorManager.address);
 
     // Assert new inbox not considered inbox before enrolled
     expect(await connectionManager.isInbox(newInbox.address)).to.be.false;
@@ -116,43 +135,19 @@ describe('AbacusConnectionManager', async () => {
     expect(await connectionManager.isInbox(enrolledInbox.address)).to.be.false;
   });
 
-  it('Owner can enroll multiple inboxes per domain', async () => {
-    const newRemoteDomain = 3000;
+  it('Owner cannot enroll multiple inboxes with same domain hash', async () => {
     const inboxFactory = new TestInbox__factory(signer);
-    const newInbox1 = await inboxFactory.deploy(localDomain);
-    const newInbox2 = await inboxFactory.deploy(localDomain);
-
-    // Assert new inbox not considered inbox before enrolled
-    expect(await connectionManager.isInbox(newInbox1.address)).to.be.false;
-    expect(await connectionManager.isInbox(newInbox2.address)).to.be.false;
-
-    await expect(
-      connectionManager.enrollInbox(newRemoteDomain, newInbox1.address),
-    ).to.emit(connectionManager, 'InboxEnrolled');
+    const newInbox = await inboxFactory.deploy(localDomain);
+    await newInbox.initialize(remoteDomain, validatorManager.address);
+    await connectionManager.unenrollInbox(enrolledInbox.address);
     await expect(
-      connectionManager.enrollInbox(newRemoteDomain, newInbox2.address),
-    ).to.emit(connectionManager, 'InboxEnrolled');
-
-    expect(await connectionManager.inboxToDomain(newInbox1.address)).to.equal(
-      newRemoteDomain,
-    );
-    expect(await connectionManager.inboxToDomain(newInbox2.address)).to.equal(
-      newRemoteDomain,
-    );
-
-    expect(await connectionManager.isInbox(newInbox1.address)).to.be.true;
-    expect(await connectionManager.isInbox(newInbox2.address)).to.be.true;
-
-    expect(await connectionManager.getInboxes(newRemoteDomain)).to.eql([
-      newInbox1.address,
-      newInbox2.address,
-    ]);
+      connectionManager.enrollInbox(remoteDomain, newInbox.address),
+    ).to.be.revertedWith('domain hash previously enrolled');
   });
 
   it('Owner cannot enroll an inbox twice', async () => {
-    const newRemoteDomain = 3000;
     await expect(
-      connectionManager.enrollInbox(newRemoteDomain, enrolledInbox.address),
+      connectionManager.enrollInbox(remoteDomain, enrolledInbox.address),
     ).to.be.revertedWith('already inbox');
   });
 });

typescript/sdk/src/core/TestCoreDeployer.ts

@@ -16,10 +16,12 @@ import {
 } from './TestCoreApp';
 import { coreFactories } from './contracts';
 
+const nonZeroAddress = ethers.constants.AddressZero.replace('00', '01');
+
 // dummy config as TestInbox and TestOutbox do not use deployed ValidatorManager
 const testValidatorManagerConfig: CoreConfig = {
   validatorManager: {
-    validators: [ethers.constants.AddressZero],
+    validators: [nonZeroAddress],
     threshold: 1,
   },
 };