STABLE-7871: (Part 1) Audit fixes (circlefin#51)

src/roles/Attestable.sol

@@ -125,13 +125,7 @@ contract Attestable is Ownable2Step {
     function updateAttesterManager(
         address newAttesterManager
     ) external onlyOwner {
-        require(
-            newAttesterManager != address(0),
-            "Invalid attester manager address"
-        );
-        address _oldAttesterManager = _attesterManager;
         _setAttesterManager(newAttesterManager);
-        emit AttesterManagerUpdated(_oldAttesterManager, newAttesterManager);
     }
 
     /**
@@ -167,25 +161,7 @@ contract Attestable is Ownable2Step {
     function setSignatureThreshold(
         uint256 newSignatureThreshold
     ) external onlyAttesterManager {
-        require(newSignatureThreshold != 0, "Invalid signature threshold");
-
-        // New signature threshold cannot exceed the number of enabled attesters
-        require(
-            newSignatureThreshold <= enabledAttesters.length(),
-            "New signature threshold too high"
-        );
-
-        require(
-            newSignatureThreshold != signatureThreshold,
-            "Signature threshold already set"
-        );
-
-        uint256 _oldSignatureThreshold = signatureThreshold;
-        signatureThreshold = newSignatureThreshold;
-        emit SignatureThresholdUpdated(
-            _oldSignatureThreshold,
-            signatureThreshold
-        );
+        _setSignatureThreshold(newSignatureThreshold);
     }
 
     /**
@@ -208,10 +184,18 @@ contract Attestable is Ownable2Step {
     // ============ Internal Utils ============
     /**
      * @dev Sets a new attester manager address
+     * @dev Emits an {AttesterManagerUpdated} event
+     * @dev Reverts if _newAttesterManager is the zero address
      * @param _newAttesterManager attester manager address to set
      */
     function _setAttesterManager(address _newAttesterManager) internal {
+        require(
+            _newAttesterManager != address(0),
+            "Invalid attester manager address"
+        );
+        address _oldAttesterManager = _attesterManager;
         _attesterManager = _newAttesterManager;
+        emit AttesterManagerUpdated(_oldAttesterManager, _newAttesterManager);
     }
 
     /**
@@ -290,4 +274,33 @@ contract Attestable is Ownable2Step {
     ) internal pure returns (address) {
         return (ECDSA.recover(_digest, _signature));
     }
+
+    /**
+     * @notice Sets the threshold of signatures required to attest to a message.
+     * (This is the m in m/n multisig.)
+     * @dev New signature threshold must be nonzero, and must not exceed number
+     * of enabled attesters.
+     * @param _newSignatureThreshold new signature threshold
+     */
+    function _setSignatureThreshold(uint256 _newSignatureThreshold) internal {
+        require(_newSignatureThreshold != 0, "Invalid signature threshold");
+
+        // New signature threshold cannot exceed the number of enabled attesters
+        require(
+            _newSignatureThreshold <= enabledAttesters.length(),
+            "New signature threshold too high"
+        );
+
+        require(
+            _newSignatureThreshold != signatureThreshold,
+            "Signature threshold already set"
+        );
+
+        uint256 _oldSignatureThreshold = signatureThreshold;
+        signatureThreshold = _newSignatureThreshold;
+        emit SignatureThresholdUpdated(
+            _oldSignatureThreshold,
+            _newSignatureThreshold
+        );
+    }
 }

src/roles/TokenController.sol

@@ -121,7 +121,7 @@ abstract contract TokenController {
      * Note:
      * - A remote token (on a certain remote domain) can only map to one local token, but many remote tokens
      * can map to the same local token.
-     * - Setting a token pair does not enable the `localToken` (that requires calling setLocalTokenEnabledStatus.)
+     * - Setting a token pair does not enable the `localToken` for deposits (that requires calling setMaxBurnAmountPerMessage.)
      */
     function linkTokenPair(
         address localToken,
@@ -214,11 +214,10 @@ abstract contract TokenController {
      * @param remoteToken Remote token
      * @return Local token address
      */
-    function _getLocalToken(uint32 remoteDomain, bytes32 remoteToken)
-        internal
-        view
-        returns (address)
-    {
+    function _getLocalToken(
+        uint32 remoteDomain,
+        bytes32 remoteToken
+    ) internal view returns (address) {
         bytes32 _remoteTokensKey = _hashRemoteDomainAndToken(
             remoteDomain,
             remoteToken
@@ -233,11 +232,10 @@ abstract contract TokenController {
      * @param remoteToken Address of remote token as bytes32
      * @return keccak hash of packed remote domain and token
      */
-    function _hashRemoteDomainAndToken(uint32 remoteDomain, bytes32 remoteToken)
-        internal
-        pure
-        returns (bytes32)
-    {
+    function _hashRemoteDomainAndToken(
+        uint32 remoteDomain,
+        bytes32 remoteToken
+    ) internal pure returns (bytes32) {
         return keccak256(abi.encodePacked(remoteDomain, remoteToken));
     }
 }

src/v2/BaseMessageTransmitter.sol

@@ -39,6 +39,10 @@ contract BaseMessageTransmitter is
      */
     event MaxMessageBodySizeUpdated(uint256 newMaxMessageBodySize);
 
+    // ============ Constants ============
+    // A constant value indicating that a nonce has been used
+    uint256 public constant NONCE_USED = 1;
+
     // ============ State Variables ============
     // Domain of chain on which the contract is deployed
     uint32 public immutable localDomain;
@@ -73,8 +77,7 @@ contract BaseMessageTransmitter is
     function setMaxMessageBodySize(
         uint256 newMaxMessageBodySize
     ) external onlyOwner {
-        maxMessageBodySize = newMaxMessageBodySize;
-        emit MaxMessageBodySizeUpdated(maxMessageBodySize);
+        _setMaxMessageBodySize(newMaxMessageBodySize);
     }
 
     /**
@@ -83,4 +86,14 @@ contract BaseMessageTransmitter is
     function initializedVersion() external view returns (uint64) {
         return _getInitializedVersion();
     }
+
+    // ============ Internal Utils ============
+    /**
+     * @notice Sets the max message body size
+     * @param _newMaxMessageBodySize new max message body size, in bytes
+     */
+    function _setMaxMessageBodySize(uint256 _newMaxMessageBodySize) internal {
+        maxMessageBodySize = _newMaxMessageBodySize;
+        emit MaxMessageBodySizeUpdated(maxMessageBodySize);
+    }
 }

src/v2/BaseTokenMessenger.sol

@@ -140,15 +140,7 @@ abstract contract BaseTokenMessenger is Rescuable, Denylistable, Initializable {
         uint32 domain,
         bytes32 tokenMessenger
     ) external onlyOwner {
-        require(tokenMessenger != bytes32(0), "bytes32(0) not allowed");
-
-        require(
-            remoteTokenMessengers[domain] == bytes32(0),
-            "TokenMessenger already set"
-        );
-
-        remoteTokenMessengers[domain] = tokenMessenger;
-        emit RemoteTokenMessengerAdded(domain, tokenMessenger);
+        _addRemoteTokenMessenger(domain, tokenMessenger);
     }
 
     /**
@@ -174,16 +166,7 @@ abstract contract BaseTokenMessenger is Rescuable, Denylistable, Initializable {
      * @param newLocalMinter The address of the minter on the local domain.
      */
     function addLocalMinter(address newLocalMinter) external onlyOwner {
-        require(newLocalMinter != address(0), "Zero address not allowed");
-
-        require(
-            address(localMinter) == address(0),
-            "Local minter is already set."
-        );
-
-        localMinter = ITokenMinterV2(newLocalMinter);
-
-        emit LocalMinterAdded(newLocalMinter);
+        _setLocalMinter(newLocalMinter);
     }
 
     /**
@@ -260,7 +243,7 @@ abstract contract BaseTokenMessenger is Rescuable, Denylistable, Initializable {
      * @return true if message sender is the registered local message transmitter
      */
     function _isLocalMessageTransmitter() internal view returns (bool) {
-        return msg.sender == address(localMessageTransmitter);
+        return msg.sender == localMessageTransmitter;
     }
 
     /**
@@ -333,4 +316,43 @@ abstract contract BaseTokenMessenger is Rescuable, Denylistable, Initializable {
         feeRecipient = _feeRecipient;
         emit FeeRecipientSet(_feeRecipient);
     }
+
+    /**
+     * @notice Sets the local minter for the local domain.
+     * @dev Reverts if a minter is already set for the local domain.
+     * @param _newLocalMinter The address of the minter on the local domain.
+     */
+    function _setLocalMinter(address _newLocalMinter) internal {
+        require(_newLocalMinter != address(0), "Zero address not allowed");
+
+        require(
+            address(localMinter) == address(0),
+            "Local minter is already set."
+        );
+
+        localMinter = ITokenMinterV2(_newLocalMinter);
+
+        emit LocalMinterAdded(_newLocalMinter);
+    }
+
+    /**
+     * @notice Add the TokenMessenger for a remote domain.
+     * @dev Reverts if there is already a TokenMessenger set for domain.
+     * @param _domain Domain of remote TokenMessenger.
+     * @param _tokenMessenger Address of remote TokenMessenger as bytes32.
+     */
+    function _addRemoteTokenMessenger(
+        uint32 _domain,
+        bytes32 _tokenMessenger
+    ) internal {
+        require(_tokenMessenger != bytes32(0), "bytes32(0) not allowed");
+
+        require(
+            remoteTokenMessengers[_domain] == bytes32(0),
+            "TokenMessenger already set"
+        );
+
+        remoteTokenMessengers[_domain] = _tokenMessenger;
+        emit RemoteTokenMessengerAdded(_domain, _tokenMessenger);
+    }
 }

src/v2/MessageTransmitterV2.sol

@@ -78,8 +78,8 @@ contract MessageTransmitterV2 is IMessageTransmitterV2, BaseMessageTransmitter {
     // ============ Initializers ============
     /**
      * @notice Initializes the contract
-     * @dev Addresses must be non-zero
-     * @dev Signature threshold must be greater than zero
+     * @dev Owner, pauser, rescuer, attesterManager, and attesters must be non-zero.
+     * @dev Signature threshold must be non-zero, but not exceed the number of enabled attesters
      * @param owner_ Owner address
      * @param pauser_ Pauser address
      * @param rescuer_ Rescuer address
@@ -102,7 +102,6 @@ contract MessageTransmitterV2 is IMessageTransmitterV2, BaseMessageTransmitter {
             attesterManager_ != address(0),
             "AttesterManager is the zero address"
         );
-        require(signatureThreshold_ > 0, "Signature threshold is zero");
         require(
             signatureThreshold_ <= attesters_.length,
             "Signature threshold exceeds attesters"
@@ -115,18 +114,20 @@ contract MessageTransmitterV2 is IMessageTransmitterV2, BaseMessageTransmitter {
         _updatePauser(pauser_);
         _setAttesterManager(attesterManager_);
 
-        // Settings
-        signatureThreshold = signatureThreshold_;
-        maxMessageBodySize = maxMessageBodySize_;
+        // Max message body size
+        _setMaxMessageBodySize(maxMessageBodySize_);
 
         // Attester configuration
         uint256 _attestersLength = attesters_.length;
         for (uint256 i; i < _attestersLength; ++i) {
             _enableAttester(attesters_[i]);
         }
 
+        // Signature threshold
+        _setSignatureThreshold(signatureThreshold_);
+
         // Claim 0-nonce
-        usedNonces[bytes32(0)] = 1;
+        usedNonces[bytes32(0)] = NONCE_USED;
     }
 
     // ============ External Functions  ============
@@ -217,7 +218,7 @@ contract MessageTransmitterV2 is IMessageTransmitterV2, BaseMessageTransmitter {
         ) = _validateReceivedMessage(message, attestation);
 
         // Mark nonce as used
-        usedNonces[_nonce] = 1;
+        usedNonces[_nonce] = NONCE_USED;
 
         // Handle receive message
         if (_finalityThresholdExecuted < FINALITY_THRESHOLD_FINALIZED) {

src/v2/TokenMessengerV2.sol

@@ -91,6 +91,7 @@ contract TokenMessengerV2 is IMessageHandlerV2, BaseTokenMessenger {
      * @dev Reverts if `remoteDomains_` and `remoteTokenMessengers_` are unequal length
      * @dev Each remoteTokenMessenger address must correspond to the remote domain at the same
      * index in respective arrays.
+     * @dev Reverts if any `remoteTokenMessengers_` entry equals bytes32(0)
      * @param owner_ Owner address
      * @param rescuer_ Rescuer address
      * @param feeRecipient_ FeeRecipient address
@@ -109,7 +110,6 @@ contract TokenMessengerV2 is IMessageHandlerV2, BaseTokenMessenger {
         bytes32[] calldata remoteTokenMessengers_
     ) external initializer {
         require(owner_ != address(0), "Owner is the zero address");
-        require(tokenMinter_ != address(0), "TokenMinter is the zero address");
         require(
             remoteDomains_.length == remoteTokenMessengers_.length,
             "Invalid remote domain configuration"
@@ -121,18 +121,16 @@ contract TokenMessengerV2 is IMessageHandlerV2, BaseTokenMessenger {
         _updateDenylister(denylister_);
         _setFeeRecipient(feeRecipient_);
 
-        localMinter = ITokenMinterV2(tokenMinter_);
+        // Local minter configuration
+        _setLocalMinter(tokenMinter_);
 
-        // Remote messenger configuration
+        // Remote token messenger configuration
         uint256 _remoteDomainsLength = remoteDomains_.length;
         for (uint256 i; i < _remoteDomainsLength; ++i) {
-            require(
-                remoteTokenMessengers_[i] != bytes32(0),
-                "Invalid TokenMessenger"
+            _addRemoteTokenMessenger(
+                remoteDomains_[i],
+                remoteTokenMessengers_[i]
             );
-            uint32 _remoteDomain = remoteDomains_[i];
-            bytes32 _remoteTokenMessenger = remoteTokenMessengers_[i];
-            remoteTokenMessengers[_remoteDomain] = _remoteTokenMessenger;
         }
     }