Feature/tob audit fixes

contracts/bridges/Accounting.sol

@@ -46,14 +46,15 @@ abstract contract Accounting is Ownable, ReentrancyGuard {
         _;
     }
 
-    /// @dev Used by parent contract to ensure that the Bonder is solvent at the end of the transaction.
+    /// @dev Used by child contract to ensure that the Bonder is solvent at the end of the transaction.
     modifier requirePositiveBalance {
         _;
         require(getCredit(msg.sender) >= getDebitAndAdditionalDebit(msg.sender), "ACT: Not enough available credit");
     }
 
     /// @dev Sets the Bonder addresses
     constructor(IBonderRegistry registry) public {
+        require(registry != IBonderRegistry(0), "ACT: Cannot set registry to zero address");
         _registry = registry;
     }
 
@@ -65,7 +66,7 @@ abstract contract Accounting is Ownable, ReentrancyGuard {
     function _transferToBridge(address from, uint256 amount) internal virtual;
 
     /**
-     * @dev This function can be optionally overridden by a parent contract to track any additional
+     * @dev This function can be optionally overridden by a child contract to track any additional
      * debit balance in an alternative way.
      */
     function _additionalDebit(address /*bonder*/) internal view virtual returns (uint256) {

contracts/bridges/BonderRegistry.sol

@@ -19,6 +19,7 @@ contract BonderRegistry is IBonderRegistry, Ownable {
 
     constructor(address[] memory bonders) public {
         for (uint256 i = 0; i < bonders.length; i++) {
+            require(bonders[i] != address(0), "BR: Cannot add zero address bonder");
             isBonder[bonders[i]] = true;
         }
     }
@@ -37,6 +38,7 @@ contract BonderRegistry is IBonderRegistry, Ownable {
      * @param bonder The address being added as a Bonder
      */
     function addBonder(address bonder) external onlyOwner {
+        require(bonder != address(0), "BR: Cannot add zero address bonder");
         require(isBonder[bonder] == false, "BR: Address is already bonder");
         isBonder[bonder] = true;
 

contracts/bridges/Bridge.sol

@@ -100,7 +100,7 @@ abstract contract Bridge is Accounting, SwapDataConsumer {
     }
 
     /**
-     * @notice getChainId can be overridden by subclasses if needed for compatibility or testing purposes.
+     * @notice getChainId can be overridden by child contracts if needed for compatibility or testing purposes.
      * @dev Get the current chainId
      * @return chainId The current chainId
      */
@@ -269,7 +269,7 @@ abstract contract Bridge is Accounting, SwapDataConsumer {
         bytes32 transferRootId = getTransferRootId(rootHash, transferRootTotalAmount);
 
         uint256 amount = _bondedWithdrawalAmounts[bonder][transferId];
-        require(amount > 0, "L2_BRG: transferId has no bond");
+        require(amount > 0, "BRG: transferId has no bond");
 
         _bondedWithdrawalAmounts[bonder][transferId] = 0;
         _addToAmountWithdrawn(transferRootId, amount);
@@ -300,6 +300,7 @@ abstract contract Bridge is Accounting, SwapDataConsumer {
                 _bondedWithdrawalAmounts[bonder][transferIds[i]] = 0;
             }
         }
+        require(totalBondsSettled > 0, "BRG: No transfer bonds to settle");
 
         bytes32 rootHash = Lib_MerkleTree.getMerkleRoot(transferIds);
         bytes32 transferRootId = getTransferRootId(rootHash, totalAmount);

contracts/bridges/L1_Bridge.sol

@@ -8,6 +8,20 @@ import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
 import "./Bridge.sol";
 import "./L2_Bridge.sol";
 
+contract IL2_Bridge is SwapDataConsumer {
+    // Redeclare distribute as payable allow value to be forwarded to xDomainConnector
+    function distribute(
+        address recipient,
+        uint256 amount,
+        SwapData calldata swapData,
+        address relayer,
+        uint256 relayerFee
+    )
+        external 
+        payable
+    {}
+}
+
 /**
  * @dev L1_Bridge is responsible for the bonding and challenging of TransferRoots. All TransferRoots
  * originate in the L1_Bridge through `bondTransferRoot` and are propagated up to destination L2s.
@@ -100,6 +114,7 @@ abstract contract L1_Bridge is Bridge {
         public
         Bridge(_registry)
     {
+        require(_token != address(0), "L1_BRG: Cannot set token to zero address");
         token = _token;
     }
 
@@ -145,7 +160,7 @@ abstract contract L1_Bridge is Bridge {
             forwardedValue = msg.value;
         }
 
-        L2_Bridge(xDomainConnector).distribute(
+        IL2_Bridge(xDomainConnector).distribute{value: forwardedValue}(
             recipient,
             amount,
             swapData,
@@ -328,7 +343,7 @@ abstract contract L1_Bridge is Bridge {
         address bonder = transferBond.bonder;
         timeSlotToAmountBonded[timeSlot][bonder] = timeSlotToAmountBonded[timeSlot][bonder].sub(bondAmount);
 
-        _addDebit(transferBond.bonder, bondAmount);
+        _addDebit(bonder, bondAmount);
 
         // Get stake for challenge
         uint256 challengeStakeAmount = getChallengeAmountForTransferAmount(originalAmount);

contracts/bridges/L2_AmmWrapper.sol

@@ -4,12 +4,14 @@ pragma solidity 0.6.12;
 pragma experimental ABIEncoderV2;
 
 import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
 import "../saddle/Swap.sol";
 import "./L2_Bridge.sol";
 import "../interfaces/IWETH.sol";
 import "./SwapDataConsumer.sol";
 
 contract L2_AmmWrapper is SwapDataConsumer {
+    using SafeERC20 for IERC20;
 
     L2_Bridge public immutable bridge;
     IERC20 public immutable l2CanonicalToken;
@@ -27,6 +29,11 @@ contract L2_AmmWrapper is SwapDataConsumer {
     )
         public
     {
+        require(_bridge != L2_Bridge(0), "L2_AMM_W: Cannot set bridge to zero address");
+        require(_l2CanonicalToken != IERC20(0), "L2_AMM_W: Cannot set l2CanonicalToken to zero address");
+        require(_hToken != IERC20(0), "L2_AMM_W: Cannot set hToken to zero address");
+        require(_exchangeAddress != Swap(0), "L2_AMM_W: Cannot set exchangeAddress to zero address");
+
         bridge = _bridge;
         l2CanonicalToken = _l2CanonicalToken;
         l2CanonicalTokenIsEth = _l2CanonicalTokenIsEth;
@@ -50,15 +57,17 @@ contract L2_AmmWrapper is SwapDataConsumer {
         payable
     {
         require(amount >= bonderFee, "L2_AMM_W: Bonder fee cannot exceed amount");
+        require(recipient != address(0), "L2_AMM_W: Recipient cannot be zero address");
+        require(bonder != address(0), "L2_AMM_W: Bonder cannot be zero address");
 
         if (l2CanonicalTokenIsEth) {
             require(msg.value == amount, "L2_AMM_W: Value does not match amount");
             IWETH(address(l2CanonicalToken)).deposit{value: amount}();
         } else {
-            require(l2CanonicalToken.transferFrom(msg.sender, address(this), amount), "L2_AMM_W: TransferFrom failed");
+            l2CanonicalToken.safeTransferFrom(msg.sender, address(this), amount);
         }
 
-        require(l2CanonicalToken.approve(address(exchangeAddress), amount), "L2_AMM_W: Approve failed");
+        l2CanonicalToken.safeApprove(address(exchangeAddress), amount);
         uint256 swapAmount = Swap(exchangeAddress).swap(
             swapData.tokenIndex,
             0,
@@ -84,8 +93,10 @@ contract L2_AmmWrapper is SwapDataConsumer {
     )
         external
     {
-        require(hToken.transferFrom(msg.sender, address(this), amount), "L2_AMM_W: TransferFrom failed");
-        require(hToken.approve(address(exchangeAddress), amount), "L2_AMM_W: Approve failed");
+        require(recipient != address(0), "L2_AMM_W: Recipient cannot be zero address");
+
+        hToken.safeTransferFrom(msg.sender, address(this), amount);
+        hToken.safeApprove(address(exchangeAddress), amount);
 
         uint256 amountOut = 0;
         try Swap(exchangeAddress).swap(
@@ -100,7 +111,7 @@ contract L2_AmmWrapper is SwapDataConsumer {
 
         if (amountOut == 0) {
             // Transfer hToken to recipient if swap fails
-            require(hToken.transfer(recipient, amount), "L2_AMM_W: Transfer failed");
+            hToken.safeTransfer(recipient, amount);
             return;
         }
 
@@ -109,7 +120,7 @@ contract L2_AmmWrapper is SwapDataConsumer {
             (bool success, ) = recipient.call{value: amountOut}(new bytes(0));
             require(success, 'L2_AMM_W: ETH transfer failed');
         } else {
-            require(l2CanonicalToken.transfer(recipient, amountOut), "L2_AMM_W: Transfer failed");
+            l2CanonicalToken.safeTransfer(recipient, amountOut);
         }
     }
 }

contracts/bridges/L2_Bridge.sol

@@ -10,6 +10,7 @@ import "./Bridge.sol";
 import "./HopBridgeToken.sol";
 import "../libraries/Lib_MerkleTree.sol";
 import "./L2_AmmWrapper.sol";
+import "./L1_Bridge.sol";
 
 /**
  * @dev The L2_Bridge is responsible for aggregating pending Transfers into TransferRoots. Each newly
@@ -21,21 +22,21 @@ abstract contract L2_Bridge is Bridge {
     using SafeERC20 for IERC20;
 
     HopBridgeToken public immutable hToken;
-    address public l1BridgeConnector;
+    address public bridgeConnector;
     L2_AmmWrapper public ammWrapper;
     mapping(uint256 => bool) public activeChainIds;
     uint256 public minimumForceCommitDelay = 1 days;
     uint256 public maxPendingTransfers = 128;
     uint256 public minBonderBps = 2;
     uint256 public minBonderFeeAbsolute = 0;
 
-    // destnation chain Id -> bonder address -> pending transfer Ids
+    // destination chain Id -> bonder address -> pending transfer Ids
     mapping(uint256 => mapping(address => bytes32[])) public pendingTransferIds;
-    // destnation chain Id -> bonder address -> pending amount
+    // destination chain Id -> bonder address -> pending amount
     mapping(uint256 => mapping(address => uint256)) public pendingAmount;
-    // destnation chain Id -> bonder address -> last commit time
+    // destination chain Id -> bonder address -> last commit time
     mapping(uint256 => mapping(address => uint256)) public lastCommitTime;
-    // destnation chain Id -> bonder address -> root index
+    // destination chain Id -> bonder address -> root index
     mapping(uint256 => mapping(address => uint256)) public rootIndex;
 
     uint256 public transferNonceIncrementer;
@@ -76,7 +77,7 @@ abstract contract L2_Bridge is Bridge {
     );
 
     modifier onlyL1Bridge {
-        require(msg.sender == l1BridgeConnector, "L2_BRG: xDomain caller must be L1 Bridge");
+        require(msg.sender == bridgeConnector, "L2_BRG: xDomain caller must be L1 Bridge");
         _;
     }
 
@@ -88,6 +89,7 @@ abstract contract L2_Bridge is Bridge {
         public
         Bridge(registry)
     {
+        require(_hToken != HopBridgeToken(0), "L2_BRG: Cannot set hToken to zero address");
         hToken = _hToken;
 
         for (uint256 i = 0; i < _activeChainIds.length; i++) {
@@ -280,21 +282,16 @@ abstract contract L2_Bridge is Bridge {
 
         lastCommitTime[destinationChainId][bonder] = block.timestamp;
         rootIndex[destinationChainId][bonder]++;
+        pendingAmount[destinationChainId][bonder] = 0;
+        delete pendingTransferIds[destinationChainId][bonder];
 
-        bytes memory confirmTransferRootMessage = abi.encodeWithSignature(
-            "confirmTransferRoot(uint256,bytes32,uint256,uint256,uint256)",
+        L1_Bridge(bridgeConnector).confirmTransferRoot(
             getChainId(),
             rootHash,
             destinationChainId,
             totalAmount,
             rootCommittedAt
         );
-
-        pendingAmount[destinationChainId][bonder] = 0;
-        delete pendingTransferIds[destinationChainId][bonder];
-
-        (bool success,) = l1BridgeConnector.call(confirmTransferRootMessage);
-        require(success, "L2_BRG: Call to L1 bridge failed");
     }
 
     function _distribute(
@@ -337,11 +334,13 @@ abstract contract L2_Bridge is Bridge {
     /* ========== External Config Management Functions ========== */
 
     function setAmmWrapper(L2_AmmWrapper _ammWrapper) external onlyOwner {
+        require(_ammWrapper != L2_AmmWrapper(0), "L2_BRG: Cannot set ammWrapper to zero address");
         ammWrapper = _ammWrapper;
     }
 
-    function setL1BridgeConnector(address _l1BridgeConnector) external onlyOwner {
-        l1BridgeConnector = _l1BridgeConnector;
+    function setBridgeConnector(address _bridgeConnector) external onlyOwner {
+        require(_bridgeConnector != address(0), "L2_BRG: Cannot set bridgeConnector to zero address");
+        bridgeConnector = _bridgeConnector;
     }
 
     function addActiveChainIds(uint256[] calldata chainIds) external onlyOwner {

contracts/bridges/SwapDataConsumer.sol

@@ -3,7 +3,7 @@
 pragma solidity 0.6.12;
 pragma experimental ABIEncoderV2;
 
-contract SwapDataConsumer {
+abstract contract SwapDataConsumer {
     struct SwapData {
         uint8 tokenIndex;
         uint256 amountOutMin;

contracts/connectors/Connector.sol

@@ -3,31 +3,33 @@
 pragma solidity 0.6.12;
 
 abstract contract Connector {
-    address public owner;
-    address public xDomainAddress;
+    address public localAddress;
+    address public xDomainConnector;
 
-    constructor(address _owner) public {
-        owner = _owner;
+    constructor(address _localAddress) public {
+        require(_localAddress != address(0), "CNR: localAddress cannot be zero address");
+        localAddress = _localAddress;
     }
 
-    fallback () external {
-        if (msg.sender == owner) {
+    fallback () external payable {
+        if (msg.sender == localAddress) {
             _forwardCrossDomainMessage();
         } else {
             _verifySender();
 
-            (bool success,) = owner.call(msg.data);
+            (bool success,) = localAddress.call(msg.data);
             require(success, "CNR: Failed to forward message");
         }
     }
 
     /**
      * @dev Sets the l2BridgeConnectorAddress
-     * @param _xDomainAddress The new bridge connector address
+     * @param _xDomainConnector The new bridge connector address
      */
-    function setXDomainAddress(address _xDomainAddress) external {
-        require(xDomainAddress == address(0), "CNR: Connector address has already been set");
-        xDomainAddress = _xDomainAddress;
+    function setxDomainConnector(address _xDomainConnector) external {
+        require(_xDomainConnector != address(0), "CNR: Cannot set xDomainConnector to zero address");
+        require(xDomainConnector == address(0), "CNR: xDomainConnector has already been set");
+        xDomainConnector = _xDomainConnector;
     }
 
     /* ========== Virtual functions ========== */

contracts/connectors/L1_ArbitrumConnector.sol

@@ -6,16 +6,17 @@ import "./Connector.sol";
 
 contract L1_ArbitrumConnector is Connector {
     constructor (
-        address _owner
+        address _localAddress
     )
         public
-        Connector(_owner)
+        Connector(_localAddress)
     {}
 
     /* ========== Override Functions ========== */
 
     function _forwardCrossDomainMessage() internal override {
         // ToDo not implemented
+        // ToDo pass msg.value to Inbox to pay for L2 fee
     }
 
     function _verifySender() internal override {

contracts/connectors/L1_OptimismConnector.sol

@@ -10,12 +10,12 @@ contract L1_OptimismConnector is Connector {
     uint256 public immutable defaultGasLimit;
 
     constructor (
-        address _owner,
+        address _localAddress,
         iOVM_L1CrossDomainMessenger _l1MessengerAddress,
         uint32 _defaultGasLimit
     )
         public
-        Connector(_owner)
+        Connector(_localAddress)
     {
         l1MessengerAddress = _l1MessengerAddress;
         defaultGasLimit = _defaultGasLimit;
@@ -25,15 +25,15 @@ contract L1_OptimismConnector is Connector {
 
     function _forwardCrossDomainMessage() internal override {
         l1MessengerAddress.sendMessage(
-            xDomainAddress,
+            xDomainConnector,
             msg.data,
             uint32(defaultGasLimit)
         );
     }
 
     function _verifySender() internal override {
         require(msg.sender == address(l1MessengerAddress), "L1_OVM_CNR: Caller is not l1MessengerAddress");
-        // Verify that cross-domain sender is xDomainAddress
-        require(l1MessengerAddress.xDomainMessageSender() == xDomainAddress, "L1_OVM_CNR: Invalid cross-domain sender");
+        // Verify that cross-domain sender is xDomainConnector
+        require(l1MessengerAddress.xDomainMessageSender() == xDomainConnector, "L1_OVM_CNR: Invalid cross-domain sender");
     }
 }