Merge pull request #124 from valory-xyz/test_guard_cm

refactor and test: addressing internal audit and adding full coverage

audits/internal10/README.md

@@ -30,6 +30,7 @@ File                                  |  % Stmts | % Branch |  % Funcs |  % Line
 --------------------------------------|----------|----------|----------|----------|----------------|
 ```
 Please, pay attention. More tests are needed and magic offsets (like MIN_ARBITRUM_PAYLOAD_LENGTH) can only be checked during testing
+[x] fixed
 
 ### Storage timelock
 Using sol2uml tools: https://github.com/naddison36/sol2uml <br>
@@ -50,6 +51,7 @@ Dubious typecast in VerifyData._verifyData(address,bytes,uint256) (ProcessBridge
 	bytes => bytes4 casting occurs in targetSelectorChainId |= uint256(uint32(bytes4(data))) << 160 (ProcessBridgedDataArbitrum-flatten.sol#30)
 	bytes => bytes4 casting occurs in revert NotAuthorized(address,bytes4,uint256)(target,bytes4(data),chainId) (ProcessBridgedDataArbitrum-flatten.sol#36)
 ```
+[x] fixed
 
 - Checking for verifierL2s is contract in set function.
 ```
@@ -61,6 +63,7 @@ because if verifierL2s[i] is EOA
 bridgeParams.verifierL2.delegatecall - always success, 
 ref: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L124
 ```
+[x] fixed
 
 - Needing fix revert message
 ```
@@ -69,9 +72,4 @@ revert("Function call reverted");
 replace to error-style message 
 revert FailedInnerCall();
 ```
-
-
-
-
-
-
+[x] fixed

contracts/multisigs/GuardCM.sol

@@ -81,6 +81,13 @@ error NoSelfCall();
 /// @param state Current proposal state.
 error NotDefeated(uint256 proposalId, ProposalState state);
 
+/// @dev Delegatecall reverted.
+error DelegateCallFailed();
+
+/// @dev Only the contract address is allowed, but the EOA account was provided.
+/// @param account Account address.
+error ContractOnly(address account);
+
 /// @dev Passed L2 chain Id is not supported.
 /// @param chainId L2 chain Id.
 error L2ChainIdNotSupported(uint256 chainId);
@@ -209,18 +216,18 @@ contract GuardCM is VerifyData {
             // Check if the data goes across the bridge
             if (bridgeParams.verifierL2 != address(0)) {
                 // Process the bridge logic
-                (bool success, bytes memory returndata) = bridgeParams.verifierL2.delegatecall(abi.encodeWithSelector(
+                (bool success, bytes memory returnData) = bridgeParams.verifierL2.delegatecall(abi.encodeWithSelector(
                     IBridgeVerifier.processBridgeData.selector, callDatas[i], bridgeParams.bridgeMediatorL2, bridgeParams.chainId));
                 // Process unsuccessful delegatecall
                 if (!success) {
                     // Get the revert message bytes
-                    if (returndata.length > 0) {
+                    if (returnData.length > 0) {
                         assembly {
-                            let returndata_size := mload(returndata)
-                            revert(add(32, returndata), returndata_size)
+                            let returnDataSize := mload(returnData)
+                            revert(add(32, returnData), returnDataSize)
                         }
                     } else {
-                        revert("Function call reverted");
+                        revert DelegateCallFailed();
                     }
                 }
             } else {
@@ -370,6 +377,11 @@ contract GuardCM is VerifyData {
                 revert ZeroAddress();
             }
 
+            // Check that the verifier is a contract
+            if (verifierL2s[i].code.length == 0) {
+                revert ContractOnly(verifierL2s[i]);
+            }
+
             // Check chain Id
             uint256 chainId = chainIds[i];
             if (chainId == 0 || chainId > MAX_CHAIN_ID) {

contracts/multisigs/VerifyData.sol

@@ -25,13 +25,14 @@ abstract contract VerifyData {
         // target occupies first 160 bits
         uint256 targetSelectorChainId = uint256(uint160(target));
         // selector occupies next 32 bits
-        targetSelectorChainId |= uint256(uint32(bytes4(data))) << 160;
+        bytes4 targetSelector = bytes4(data);
+        targetSelectorChainId |= uint256(uint32(targetSelector)) << 160;
         // chainId occupies next 64 bits
         targetSelectorChainId |= chainId << 192;
 
         // Check the authorized combination of target and selector
         if (!mapAllowedTargetSelectorChainIds[targetSelectorChainId]) {
-            revert NotAuthorized(target, bytes4(data), chainId);
+            revert NotAuthorized(target, targetSelector, chainId);
         }
     }
 }

contracts/multisigs/bridge_verifier/ProcessBridgedDataArbitrum.sol

@@ -13,22 +13,17 @@ error IncorrectDataLength(uint256 expected, uint256 provided);
 /// @param chainId Chain Id.
 error WrongSelector(bytes4 functionSig, uint256 chainId);
 
-/// @dev Provided wrong L2 bridge mediator address.
-/// @param provided Provided address.
-/// @param expected Expected address.
-error WrongL2BridgeMediator(address provided, address expected);
-
 /// @title ProcessBridgedDataArbitrum - Smart contract for verifying the Guard CM bridged data on Arbitrum
 /// @author Aleksandr Kuperman - <[email protected]>
 /// @author Andrey Lebedev - <[email protected]>
 /// @author Mariapia Moscatiello - <[email protected]>
 contract ProcessBridgedDataArbitrum is VerifyBridgedData {
     // unsafeCreateRetryableTicket selector in bridge mediator L1
-    bytes4 public constant CREATE_TICKET_UNSAFE = bytes4(keccak256(bytes("unsafeCreateRetryableTicket(address,uint256,uint256,address,address,uint256,uint256,uint256,bytes)")));
+    bytes4 public constant CREATE_TICKET_UNSAFE = bytes4(keccak256(bytes("unsafeCreateRetryableTicket(address,uint256,uint256,address,address,uint256,uint256,bytes)")));
     // createRetryableTicket selector in bridge mediator L1
-    bytes4 public constant CREATE_TICKET = bytes4(keccak256(bytes("createRetryableTicket(address,uint256,uint256,address,address,uint256,uint256,uint256,bytes)")));
+    bytes4 public constant CREATE_TICKET = bytes4(keccak256(bytes("createRetryableTicket(address,uint256,uint256,address,address,uint256,uint256,bytes)")));
     // Minimum payload length for message on Arbitrum accounting for all required encoding and at least one selector
-    uint256 public constant MIN_ARBITRUM_PAYLOAD_LENGTH = 584;
+    uint256 public constant MIN_ARBITRUM_PAYLOAD_LENGTH = 324;
 
     /// @dev Processes bridged data: checks the header and verifies the payload.
     /// @param data Full data bytes with the header.
@@ -57,8 +52,8 @@ contract ProcessBridgedDataArbitrum is VerifyBridgedData {
         }
 
         // Decode the payload depending on the selector
-        (address targetAddress, , , , , , , , bytes memory targetPayload) =
-            abi.decode(payload, (address, uint256, uint256, address, address, uint256, uint256, uint256, bytes));
+        (address targetAddress, , , , , , , bytes memory targetPayload) =
+            abi.decode(payload, (address, uint256, uint256, address, address, uint256, uint256, bytes));
 
         // Verify the scope of the data
         _verifyData(targetAddress, targetPayload, chainId);

contracts/multisigs/bridge_verifier/ProcessBridgedDataOptimism.sol

@@ -28,7 +28,7 @@ contract ProcessBridgedDataOptimism is VerifyBridgedData {
     // processMessageFromSource selector (Optimism and Base chains)
     bytes4 public constant PROCESS_MESSAGE_FROM_SOURCE = bytes4(keccak256(bytes("processMessageFromSource(bytes)")));
     // Minimum payload length for message on Optimism accounting for all required encoding and at least one selector
-    uint256 public constant MIN_OPTIMISM_PAYLOAD_LENGTH = 264;
+    uint256 public constant MIN_OPTIMISM_PAYLOAD_LENGTH = 292;
 
     /// @dev Processes bridged data: checks the header and verifies the payload.
     /// @param data Full data bytes with the header.

contracts/multisigs/bridge_verifier/ProcessBridgedDataWormhole.sol

@@ -28,7 +28,7 @@ contract ProcessBridgedDataWormhole is VerifyBridgedData {
     // sendPayloadToEvm selector in bridge mediator L1
     bytes4 public constant SEND_MESSAGE = bytes4(keccak256(bytes("sendPayloadToEvm(uint16,address,bytes,uint256,uint256)")));
     // Minimum payload length for message sent via Wormhole accounting for all required encoding and at least one selector
-    uint256 public constant MIN_OPTIMISM_PAYLOAD_LENGTH = 520;
+    uint256 public constant MIN_OPTIMISM_PAYLOAD_LENGTH = 324;
 
     /// @dev Processes bridged data: checks the header and verifies the payload.
     /// @param data Full data bytes with the header.