[Governance] Audit changes

.prettierrc

@@ -4,13 +4,15 @@
   "singleQuote": true,
   "printWidth": 80,
   "arrowParens": "always",
+  "plugins": ["prettier-plugin-solidity"],
   "overrides": [
     {
       "files": "*.sol",
       "options": {
         "semi": true,
         "printWidth": 80,
-        "explicitTypes": "always"
+        "explicitTypes": "always",
+        "parser": "solidity-parse"
       }
     }
   ]

packages/core/contracts/governance/CrossChainGovernorCountingSimple.sol

@@ -58,7 +58,8 @@ abstract contract CrossChainGovernorCountingSimple is Governor, Ownable {
      *   @param proposalId id of a proposal that will use the snapshot.
      */
     function createSnapshot(uint256 proposalId) internal {
-        for (uint256 i = 1; i <= spokeContracts.length; ++i) {
+        uint256 spokeContractsLength = spokeContracts.length;
+        for (uint256 i = 1; i <= spokeContractsLength; ++i) {
             CrossChainAddress memory addressToSnapshot = spokeContracts[i - 1];
             spokeContractsMappingSnapshots[proposalId][
                 addressToSnapshot.contractAddress

packages/core/contracts/governance/DAOSpokeContract.sol

@@ -4,6 +4,8 @@ pragma solidity ^0.8.0;
 import '@openzeppelin/contracts/utils/Timers.sol';
 import '@openzeppelin/contracts/utils/Checkpoints.sol';
 import '@openzeppelin/contracts/governance/utils/IVotes.sol';
+import '@openzeppelin/contracts/utils/Address.sol';
+
 import './MetaHumanGovernor.sol';
 import './wormhole/IWormholeRelayer.sol';
 import './wormhole/IWormholeReceiver.sol';
@@ -15,6 +17,7 @@ import './magistrate/Magistrate.sol';
  *   It integrates with the MetaHumanGovernor contract for governance operations.
  */
 contract DAOSpokeContract is IWormholeReceiver, Magistrate {
+    using Address for address payable;
     bytes32 public immutable hubContractAddress;
     uint16 public immutable hubContractChainId;
     IVotes public immutable token;
@@ -87,7 +90,7 @@ contract DAOSpokeContract is IWormholeReceiver, Magistrate {
      * @dev Allows the magistrate address to withdraw all funds from the contract
      */
     function withdrawFunds() public onlyMagistrate {
-        payable(msg.sender).transfer(address(this).balance);
+        payable(msg.sender).sendValue(address(this).balance);
     }
 
     function hasVoted(

packages/core/contracts/governance/MetaHumanGovernor.sol

@@ -6,6 +6,7 @@ import '@openzeppelin/contracts/governance/extensions/GovernorSettings.sol';
 import '@openzeppelin/contracts/governance/extensions/GovernorVotes.sol';
 import '@openzeppelin/contracts/governance/extensions/GovernorVotesQuorumFraction.sol';
 import '@openzeppelin/contracts/governance/extensions/GovernorTimelockControl.sol';
+import '@openzeppelin/contracts/utils/Address.sol';
 import './CrossChainGovernorCountingSimple.sol';
 import './DAOSpokeContract.sol';
 import './wormhole/IWormholeRelayer.sol';
@@ -30,6 +31,17 @@ contract MetaHumanGovernor is
     Magistrate,
     IWormholeReceiver
 {
+    using Address for address payable;
+
+    error MessageAlreadyProcessed();
+    error OnlyRelayerAllowed();
+    error InvalidIntendedRecipient();
+    error ProposalAlreadyInitialized();
+    error CollectionPhaseUnfinished();
+    error RequestAfterVotePeriodOver();
+    error CollectionPhaseAlreadyStarted();
+    error OnlyMessagesFromSpokeReceived();
+
     IWormholeRelayer public immutable wormholeRelayer;
     uint256 internal constant GAS_LIMIT = 500_000;
     uint256 public immutable secondsPerBlock;
@@ -77,7 +89,66 @@ contract MetaHumanGovernor is
      * @dev Allows the magistrate address to withdraw all funds from the contract
      */
     function withdrawFunds() public onlyMagistrate {
-        payable(msg.sender).transfer(address(this).balance);
+        payable(msg.sender).sendValue(address(this).balance);
+    }
+
+    function cancel(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) public virtual override(Governor, IGovernor) returns (uint256) {
+        // First, perform the original cancellation logic.
+        uint256 proposalId = super.cancel(
+            targets,
+            values,
+            calldatas,
+            descriptionHash
+        );
+
+        //Notify all spoke chains about the cancellation
+        _notifySpokeChainsOfCancellation(proposalId);
+
+        return proposalId;
+    }
+
+    function _notifySpokeChainsOfCancellation(uint256 proposalId) internal {
+        uint256 spokeContractsLength = spokeContractsSnapshots[proposalId]
+            .length;
+        for (uint16 i = 0; i < spokeContractsLength; i++) {
+            bytes memory message = abi.encode(
+                2, // "2" is an inused function selector indicating cancellation
+                proposalId
+            );
+
+            bytes memory payload = abi.encode(
+                spokeContractsSnapshots[proposalId][i].contractAddress,
+                spokeContractsSnapshots[proposalId][i].chainId,
+                bytes32(uint256(uint160(address(this)))),
+                message
+            );
+
+            uint256 cost = quoteCrossChainMessage(
+                spokeContractsSnapshots[proposalId][i].chainId,
+                0
+            );
+
+            // Send cancellation message
+            wormholeRelayer.sendPayloadToEvm{value: cost}(
+                spokeContractsSnapshots[proposalId][i].chainId,
+                address(
+                    uint160(
+                        uint256(
+                            spokeContractsSnapshots[proposalId][i]
+                                .contractAddress
+                        )
+                    )
+                ),
+                payload,
+                0,
+                GAS_LIMIT
+            );
+        }
     }
 
     /**
@@ -96,9 +167,13 @@ contract MetaHumanGovernor is
         uint16 sourceChain,
         bytes32 deliveryHash // this can be stored in a mapping deliveryHash => bool to prevent duplicate deliveries
     ) public payable override {
-        require(msg.sender == address(wormholeRelayer), 'Only relayer allowed');
+        if (msg.sender != address(wormholeRelayer)) {
+            revert OnlyRelayerAllowed();
+        }
 
-        require(!processedMessages[deliveryHash], 'Message already processed');
+        if (processedMessages[deliveryHash]) {
+            revert MessageAlreadyProcessed();
+        }
 
         (
             address intendedRecipient, //chainId
@@ -108,10 +183,14 @@ contract MetaHumanGovernor is
             bytes memory decodedMessage
         ) = abi.decode(payload, (address, uint16, address, bytes));
 
-        require(
-            intendedRecipient == address(this),
-            'Message is not addressed for this contract'
-        );
+        if (intendedRecipient != address(this)) {
+            revert InvalidIntendedRecipient();
+        }
+
+        // require(
+        //     intendedRecipient == address(this),
+        //     'Message is not addressed for this contract'
+        // );
 
         processedMessages[deliveryHash] = true;
         // Gets a function selector option
@@ -149,6 +228,14 @@ contract MetaHumanGovernor is
             uint256 _abstain
         ) = abi.decode(payload, (uint16, uint256, uint256, uint256, uint256));
 
+        if (
+            !spokeContractsMappingSnapshots[_proposalId][emitterAddress][
+                emitterChainId
+            ]
+        ) {
+            revert OnlyMessagesFromSpokeReceived();
+        }
+
         require(
             spokeContractsMappingSnapshots[_proposalId][emitterAddress][
                 emitterChainId
@@ -160,7 +247,7 @@ contract MetaHumanGovernor is
         if (
             spokeVotes[_proposalId][emitterAddress][emitterChainId].initialized
         ) {
-            revert('Already initialized!');
+            revert ProposalAlreadyInitialized();
         } else {
             // Add it to the map (while setting initialized true)
             spokeVotes[_proposalId][emitterAddress][
@@ -188,10 +275,9 @@ contract MetaHumanGovernor is
     ) internal override {
         _finishCollectionPhase(proposalId);
 
-        require(
-            collectionFinished[proposalId],
-            'Collection phase for this proposal is unfinished!'
-        );
+        if (!collectionFinished[proposalId]) {
+            revert CollectionPhaseUnfinished();
+        }
 
         super._beforeExecute(
             proposalId,
@@ -227,14 +313,13 @@ contract MetaHumanGovernor is
      *  @param proposalId The ID of the proposal.
      */
     function requestCollections(uint256 proposalId) public payable {
-        require(
-            block.number > proposalDeadline(proposalId),
-            'Cannot request for vote collection until after the vote period is over!'
-        );
-        require(
-            !collectionStarted[proposalId],
-            'Collection phase for this proposal has already started!'
-        );
+        if (block.number < proposalDeadline(proposalId)) {
+            revert RequestAfterVotePeriodOver();
+        }
+
+        if (collectionStarted[proposalId]) {
+            revert CollectionPhaseAlreadyStarted();
+        }
 
         collectionStarted[proposalId] = true;
 
@@ -273,6 +358,8 @@ contract MetaHumanGovernor is
                 payload,
                 sendMessageToHubCost, // send value to enable the spoke to send back vote result
                 GAS_LIMIT
+                // spokeContracts[i-1].chainId, // refund chain
+                // address(this) // target where the refund is sent
             );
         }
     }
@@ -348,6 +435,8 @@ contract MetaHumanGovernor is
                     payload,
                     0, // no receiver value needed
                     GAS_LIMIT
+                    // chainId, // refund chain
+                    // address(this) // target where the refund is sent
                 );
             }
         }

packages/core/test/MetaHumanGovernor.ts

@@ -935,9 +935,7 @@ describe.only('MetaHumanGovernor', function () {
           await wormholeMockForDaoSpoke.getAddress()
         )
       )
-    ).to.be.revertedWith(
-      'Only messages from the spoke contracts can be received!'
-    );
+    ).to.be.revertedWithCustomError(governor, 'OnlyMessagesFromSpokeReceived');
   });
 
   it('Should receive message', async function () {
@@ -1031,7 +1029,7 @@ describe.only('MetaHumanGovernor', function () {
           await daoSpoke.getAddress()
         )
       )
-    ).to.be.revertedWith('Message already processed');
+    ).to.be.revertedWithCustomError(governor, 'MessageAlreadyProcessed');
   });
 
   it('should reverts to receive message when intended receipient is not different', async function () {
@@ -1075,7 +1073,7 @@ describe.only('MetaHumanGovernor', function () {
           await daoSpoke.getAddress()
         )
       )
-    ).to.be.revertedWith('Message is not addressed for this contract');
+    ).to.be.revertedWithCustomError(governor, 'InvalidIntendedRecipient');
   });
 
   it('Should finish collection phase', async function () {
@@ -1126,9 +1124,9 @@ describe.only('MetaHumanGovernor', function () {
     // Mine 2 blocks
     await mineNBlocks(2);
 
-    await expect(governor.requestCollections(proposalId)).to.be.revertedWith(
-      'Cannot request for vote collection until after the vote period is over!'
-    );
+    await expect(
+      governor.requestCollections(proposalId)
+    ).to.be.revertedWithCustomError(governor, 'RequestAfterVotePeriodOver');
   });
 
   it('Should fails to request collections when collection already started', async function () {
@@ -1147,9 +1145,7 @@ describe.only('MetaHumanGovernor', function () {
 
     await expect(
       governor.requestCollections(proposalId, { value: 100 })
-    ).to.be.revertedWith(
-      'Collection phase for this proposal has already started!'
-    );
+    ).to.be.revertedWithCustomError(governor, 'CollectionPhaseAlreadyStarted');
   });
 
   it('Should vote on proposal with reason', async function () {