Nomad

``` - In `contracts-core` - `Home.sol:update`: there should be check on `_newRoot != _committedRoot` - `Home.sol:setUpdater` and `Replica.sol:setUpdater`: should check the `_updater` is not zero address. Furthermore, we can add validation that the updater should not be a contract, as the signature of the updater is required. - `Replica.sol:update`: there should be check on `_newRoot != _committedRoot`. Updater can try to override the `confirmAt` timestamp in `Replica.sol:L153` without this check. - `XAppConnectionManager.sol:unenrollReplica`: - `_domain` should not be 0 - `_updater` should not be empty `bytes32.` - `_signature` should not be empty `bytes.` - `XAppConnectionManager.sol:setHome`: - `_home` should not be zero address - `XAppConnectionManager.sol:ownerEnrollReplica`: - `_replica` should not be zero address - `_domain` should not be 0 - `_domain` should be checked with the public variable `remoteDomain` of the replica. e.g. `Replica(_replica).remoteDomain() == _domain`. - `XAppConnectionManager.sol:ownerUnenrollReplica`: - `_replica` should not be zero address - `XAppConnectionManager.sol:setWatcherPermission`: - `_watcher` should not be zero address - `_domain` should not be 0 - `governance/GovernanceRouter.sol:transferRecoveryManager`: should check `_newRecoveryManager` is not zero address. - `governance/GovernanceRouter.sol:initialize`: should check `_xAppConnectionManager` and `_recoveryManager` is not zero address - `upgrade/UpgradeBeacon.sol:constructor`: should check `_controller` is a smart contract. - In `contracts-bridge` - `BridgeRouter.sol:send` should check `_destination` not being 0 - `BridgeRouter.sol:enrollCustom` should check `_domain` is not 0 and `_id` is not empty. Also, it should check that the input `_domain` is not the local domain. - `BridgeRouter.sol:migrate` should check `_oldRepr` is not zero address - `BridgeRouter.sol:preFill` should check `_origin` not being 0 - In `contracts-router` - `Router.sol:enrollRemoteRouter` should check `_domain` is not 0, and `_router` is not empty bytes - In `zodiac-module-gnomad` - `GnomadModule.sol:constructor` - check `_avatar`, `_target`, `_manager` is contract. (e.g., with open-zeppelin library: [doc](https://docs.openzeppelin.com/contracts/4.x/api/utils#Address-isContract-address-)) - check `_controller` is not a zero address - check `_controllerDomain` is not zero - `GnomadModule.sol:setUp - check `_avatar`, `_target`, `_manager` is contract - check `_controller` is not a zero address - check `_controllerDomain` is not zero - `GnomadModule.sol:onlyValid`: check that `_sender` starts with only padding 0s for the first 12 bytes. - `GnomadModule.sol:setManager`: check `_manager` is contract - `GnomadModule.sol:setController` - check `_controller` is not a zero address - check `_controllerDomain` is not zero ```

The `onlyGovernor` and the `onlyGovernorOrRecoveryManager` modifiers check that the `msg.sender` is an address registered as `governor` (or `recoveryManager`) or if the address is the `GovernanceRouter` contract itself. However, the `_callLocal` method is using Solidity's low-level call function, so methods dispatched from it will have the `GovernanceRouter` as the `msg.sender`. While we couldn't find any invalid use, governance calls executed through `_callLocal` could be used to bypass the `onlyGovernor` or `onlyGovernorOrRecoveryManager` modifiers.

On top of the regular way of sending messages, governance actions need an external agent to call the `executeCallBatch` method. If this external agent does not call this method, the batch of actions will stay forever in `Pending` status and the governance actions will never be executed.

The `setDetails` method allows anyone to change the token information (name, symbol, and decimals) as long as the new details match the `detailsHash` set by the owner. This means that, with the owner's consent, it's possible to change the decimals of the token. If mishandled (mistakenly or maliciously), this could have serious consequences, as the decimals are needed to perform proper token math.

Most methods in the `GovernanceRouter` contract are annotated with modifiers to prevent them from being executed when the contract is in recovery mode. However, the `executeCallBatch` is not annotated with any of these modifiers; it can be called regardless of the recovery status. It is unclear if this is intentional or if it's missing the right modifier. **Note**: While this method could bypass the recovery mode, the calls to execute originated from the governor router, so they are unlikely to be malicious.

The function `Replica.sol:prove` accepts the input `_leaf` and checks if it is part of the Merkle tree. Nomad architecture uses a sparse Merkle tree, in which all the non-used leaves default with empty `bytes32`. This nature of the sparse Merkle tree makes it possible for one to pass an empty `bytes32` as the `_leaf` and some artificial Merkle proof with a specified index to pass the inclusion check. The “empty leaf” message status can later be flagged as `PROVEN`, resulting in the `messages` mapping in an undesired state.

In the `improperUpdate` function, we are taking the `_signature` as a parameter to verify the correctness of the transaction. The issue here is that the signature is not associated to any nonce or address. Thus, the signature can be replayed since there is no nonce associated.

The contracts have a hard-coded “domain” value set on deployment. The hardcoded domains will work until the chain is hard forked. Once a hard fork occurs, the signature of both chains will be considered valid on both the original and the hard-forked chain.

The attacker can replay the message's signature if there are multiple deployments in the same chain running with the same updater. In the worst case, one can use it to call `Home.sol:improperUpdate` to fail the home contract.

`BridgeToken.sol:permit` function receives the signature and uses `ecrecover` to get the recovered address to verify the signature. However, the `ecrecover` call does not protect from the malleable signatures of ECDSA (see: [SWC-117](https://swcregistry.io/docs/SWC-117)). As a result, it will be possible for another user to replay the call with a different signature even without holding the address's private key and still passes the verification. Fortunately, the current implementation includes a `nonce` which will increase each time when building the `digest` of the signature. The immediate increase in the nonce avoids replay attacks with this vulnerability.