chore: address Veridise findings (#272)

contracts/handlers/DefaultMessageReceiver.sol

@@ -57,6 +57,18 @@ contract DefaultMessageReceiver is ISygmaMessageReceiver, AccessControl, ERC721H
         }
     }
 
+    /**
+        @notice Users have to understand the design and limitations behind the Actions processing.
+        The contract will try to return all the leftover tokens and native token to the
+        receiver address. This logic is applied to the native token if there was a balance
+        increase during the message processing, then to the tokenSent which is received from
+        Sygma proposal and finally to every Action.tokenReceive. In the vast majority of
+        cases that would be enough, though user can come up with a scenario where an Action
+        produces results in a receival of more than one token, while only one could be
+        specified in this particular Action.tokenReceive property. In such a case it is
+        a users responsibility to either send it all with a transferBalanceAction() Action or to
+        include an extra action[s] with tokenReceive set to each of the tokens received.
+     */
     function handleSygmaMessage(
         address tokenSent,
         uint256 amount,
@@ -128,7 +140,8 @@ contract DefaultMessageReceiver is ISygmaMessageReceiver, AccessControl, ERC721H
 
         uint256 numActions = actions.length;
         for (uint256 i = 0; i < numActions; i++) {
-            if (!isContract(actions[i].callTo)) revert InvalidContract();
+            // Allow EOA if the data is empty. Could be used to send native currency.
+            if (!isContract(actions[i].callTo) && actions[i].data.length > 0) revert InvalidContract();
             uint256 nativeValue = actions[i].nativeValue;
             if (nativeValue > 0 && address(this).balance < nativeValue) {
                 revert InsufficientNativeBalance();
@@ -170,6 +183,17 @@ contract DefaultMessageReceiver is ISygmaMessageReceiver, AccessControl, ERC721H
         }
     }
 
+    /// @notice Helper function that could be used as an Action to itself to transfer whole
+    /// @notice balance of a particular token.
+    function transferBalanceAction(address token, address receiver) external {
+        if (msg.sender != address(this)) revert InsufficientPermission();
+        if (token != zeroAddress) {
+            transferBalance(token, receiver);
+        } else {
+            transferNativeBalance(payable(receiver));
+        }
+    }
+
     function isContract(address contractAddr) internal view returns (bool) {
         uint256 size;
         assembly {

contracts/handlers/DepositDataHelper.sol

@@ -10,6 +10,8 @@ import "../interfaces/ISygmaMessageReceiver.sol";
     @author ChainSafe Systems.
  */
 contract DepositDataHelper is ERCHandlerHelpers {
+    using SanityChecks for *;
+
     address public immutable _defaultMessageReceiver;
     uint16 internal constant maxReturnBytes = 256;
     address internal constant transformRecipient = address(0);
@@ -54,13 +56,14 @@ contract DepositDataHelper is ERCHandlerHelpers {
         uint256 lenDestinationRecipientAddress;
 
         (amount, lenDestinationRecipientAddress) = abi.decode(data, (uint256, uint256));
-        address recipientAddress = address(bytes20(bytes(data[64:64 + lenDestinationRecipientAddress])));
+        lenDestinationRecipientAddress.mustBe(20);
+        address recipientAddress = address(bytes20(bytes(data[64:84])));
 
         address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
         uint256 externalAmount = convertToExternalBalance(tokenAddress, amount);
 
         // Optional message recipient transformation.
-        uint256 pointer = 64 + lenDestinationRecipientAddress;
+        uint256 pointer = 84;
         uint256 gas;
         uint256 messageLength;
         bytes memory message;

contracts/handlers/ERC1155Handler.sol

@@ -10,6 +10,7 @@ import "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol";
 import "@openzeppelin/contracts/token/ERC1155/extensions/IERC1155MetadataURI.sol";
 
 contract ERC1155Handler is IHandler, ERCHandlerHelpers, ERC1155Safe, ERC1155Holder {
+    using SanityChecks for *;
     using ERC165Checker for address;
 
     bytes private constant EMPTY_BYTES = "";
@@ -70,6 +71,7 @@ contract ERC1155Handler is IHandler, ERCHandlerHelpers, ERC1155Safe, ERC1155Hold
 
         (tokenIDs, amounts, recipient, transferData) = abi.decode(data, (uint[], uint[], bytes, bytes));
 
+        recipient.length.mustBe(20);
         bytes20 recipientAddress;
 
         assembly {
@@ -101,6 +103,7 @@ contract ERC1155Handler is IHandler, ERCHandlerHelpers, ERC1155Safe, ERC1155Hold
 
         (tokenAddress, recipient, tokenIDs, amounts, transferData) = abi.decode(data, (address, address, uint[], uint[], bytes));
 
+        recipient.mustNotBeZero();
         releaseBatchERC1155(tokenAddress, address(this), recipient, tokenIDs, amounts, transferData);
     }
 

contracts/handlers/ERC20Handler.sol

@@ -14,6 +14,7 @@ import "../utils/ExcessivelySafeCall.sol";
     @notice This contract is intended to be used with the Bridge contract.
  */
 contract ERC20Handler is IHandler, ERCHandlerHelpers, DepositDataHelper, ERC20Safe {
+    using SanityChecks for *;
     using ExcessivelySafeCall for address;
 
     error OptionalMessageCallFailed();
@@ -41,7 +42,7 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, DepositDataHelper, ERC20Sa
         optionalMessage                    bytes   bytes (160 + len(destinationRecipientAddress)) - END
         @dev Depending if the corresponding {tokenAddress} for the parsed {resourceID} is
         marked true in {_tokenContractAddressToTokenProperties[tokenAddress].isBurnable}, deposited tokens will be burned, if not, they will be locked.
-        @return an empty data.
+        @return 32-length byte array with internal bridge amount OR empty byte array if conversion is not needed.
      */
     function deposit(
         bytes32 resourceID,
@@ -60,7 +61,7 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, DepositDataHelper, ERC20Sa
             lockERC20(tokenAddress, depositor, address(this), amount);
         }
 
-        return abi.encodePacked(convertToInternalBalance(tokenAddress, amount));
+        return convertToInternalBalance(tokenAddress, amount);
     }
 
     /**
@@ -117,6 +118,7 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, DepositDataHelper, ERC20Sa
 
         (tokenAddress, recipient, amount) = abi.decode(data, (address, address, uint));
 
+        recipient.mustNotBeZero();
         releaseERC20(tokenAddress, recipient, amount);
     }
 
@@ -127,9 +129,11 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, DepositDataHelper, ERC20Sa
         Sets decimals value for contractAddress if value is provided in args.
         @param resourceID ResourceID to be used when making deposits.
         @param contractAddress Address of contract to be called when a deposit is made and a deposited is executed.
-        @param args Additional data to be passed to specified handler.
+        @param args Byte array which is either empty if the token contract decimals are the same as the bridge defaultDecimals,
+                    or has a first byte set to the uint8 decimals value of the token contract.
      */
     function setResource(bytes32 resourceID, address contractAddress, bytes calldata args) external onlyBridge {
+        contractAddress.mustNotBeZero();
         _setResource(resourceID, contractAddress);
 
         if (args.length > 0) {

contracts/handlers/ERC721Handler.sol

@@ -15,6 +15,7 @@ import "@openzeppelin/contracts/token/ERC721/extensions/IERC721Metadata.sol";
     @notice This contract is intended to be used with the Bridge contract.
  */
 contract ERC721Handler is IHandler, ERCHandlerHelpers, ERC721Safe {
+    using SanityChecks for *;
     using ERC165Checker for address;
 
     bytes4 private constant _INTERFACE_ERC721_METADATA = 0x5b5e139f;
@@ -91,7 +92,8 @@ contract ERC721Handler is IHandler, ERCHandlerHelpers, ERC721Safe {
         bytes memory metaData;
 
         (tokenID, lenDestinationRecipientAddress) = abi.decode(data, (uint, uint));
-        offsetMetaData = 64 + lenDestinationRecipientAddress;
+        lenDestinationRecipientAddress.mustBe(20);
+        offsetMetaData = 84;
         destinationRecipientAddress = bytes(data[64:offsetMetaData]);
         lenMetaData = abi.decode(data[offsetMetaData:], (uint));
         metaData = bytes(data[offsetMetaData + 32:offsetMetaData + 32 + lenMetaData]);
@@ -128,6 +130,7 @@ contract ERC721Handler is IHandler, ERCHandlerHelpers, ERC721Safe {
 
         (tokenAddress, recipient, tokenID) = abi.decode(data, (address, address, uint));
 
+        recipient.mustNotBeZero();
         releaseERC721(tokenAddress, address(this), recipient, tokenID);
     }
 
@@ -140,6 +143,7 @@ contract ERC721Handler is IHandler, ERCHandlerHelpers, ERC721Safe {
         @param args Additional data to be passed to specified handler.
      */
     function setResource(bytes32 resourceID, address contractAddress, bytes calldata args) external onlyBridge {
+        contractAddress.mustNotBeZero();
         _setResource(resourceID, contractAddress);
     }
 }

contracts/handlers/ERCHandlerHelpers.sol

@@ -3,6 +3,7 @@
 pragma solidity 0.8.11;
 
 import "../interfaces/IERCHandler.sol";
+import "../utils/SanityChecks.sol";
 
 /**
     @title Function used across handler contracts.
@@ -93,10 +94,9 @@ contract ERCHandlerHelpers is IERCHandler {
     }
 
     /**
-        @notice Converts token amount based on decimal places difference between the nework
-        deposit is made on and bridge.
+        @notice Converts token amount based on decimal places difference from bridge to current network.
         @param tokenAddress Address of contract to be used when executing proposals.
-        @param amount Decimals value to be set for {contractAddress}.
+        @param amount The bridge internal amount with defaultDecimals.
     */
     function convertToExternalBalance(address tokenAddress, uint256 amount) internal view returns(uint256) {
         Decimals memory decimals = _tokenContractAddressToTokenProperties[tokenAddress].decimals;
@@ -110,18 +110,18 @@ contract ERCHandlerHelpers is IERCHandler {
     }
 
     /**
-        @notice Converts token amount based on decimal places difference between the bridge and nework
-        deposit is executed on.
+        @notice Converts token amount based on decimal places difference from current network to bridge.
         @param tokenAddress Address of contract to be used when executing proposals.
-        @param amount Decimals value to be set for {contractAddress}.
+        @param amount The token amount with decimals on the current network.
+        @return 32-length byte array with internal bridge amount OR empty byte array if conversion is not needed.
     */
     function convertToInternalBalance(address tokenAddress, uint256 amount) internal view returns(bytes memory) {
         Decimals memory decimals = _tokenContractAddressToTokenProperties[tokenAddress].decimals;
         uint256 convertedBalance;
         if (!decimals.isSet) {
             return "";
         } else if (decimals.externalDecimals >= defaultDecimals) {
-            convertedBalance =  amount / (10 ** (decimals.externalDecimals - defaultDecimals));
+            convertedBalance = amount / (10 ** (decimals.externalDecimals - defaultDecimals));
         } else {
             convertedBalance = amount * (10 ** (defaultDecimals - decimals.externalDecimals));
         }

contracts/handlers/GmpHandler.sol

@@ -3,13 +3,16 @@
 pragma solidity 0.8.11;
 
 import "../interfaces/IHandler.sol";
+import "../utils/SanityChecks.sol";
 
 /**
     @title Handles generic deposits and deposit executions.
     @author ChainSafe Systems.
     @notice This contract is intended to be used with the Bridge contract.
  */
 contract GmpHandler is IHandler {
+    using SanityChecks for *;
+
     uint256 public constant MAX_FEE = 1000000;
 
     address public immutable _bridgeAddress;
@@ -165,10 +168,13 @@ contract GmpHandler is IHandler {
 
         maxFee                            = uint256(bytes32(data[:pointer += 32])); 
         lenExecuteFuncSignature           = uint16(bytes2(data[pointer:pointer += 2]));
+        lenExecuteFuncSignature.mustBe(4);
         executeFuncSignature              = bytes4(data[pointer:pointer += lenExecuteFuncSignature]);
         lenExecuteContractAddress         = uint8(bytes1(data[pointer:pointer += 1]));
+        lenExecuteContractAddress.mustBe(20);
         executeContractAddress            = address(uint160(bytes20(data[pointer:pointer += lenExecuteContractAddress])));
         lenExecutionDataDepositor         = uint8(bytes1(data[pointer:pointer += 1]));
+        lenExecutionDataDepositor.mustBe(20);
         executionDataDepositor            = address(uint160(bytes20(data[pointer:pointer += lenExecutionDataDepositor])));
         executionData                     = bytes(data[pointer:]);
 

contracts/handlers/NativeTokenHandler.sol

@@ -14,6 +14,7 @@ import "../utils/ExcessivelySafeCall.sol";
     @notice This contract is intended to be used with the Bridge contract.
  */
 contract NativeTokenHandler is IHandler, ERCHandlerHelpers, DepositDataHelper {
+    using SanityChecks for *;
     using ExcessivelySafeCall for address;
 
     uint256 internal constant defaultGas = 50000;
@@ -52,7 +53,7 @@ contract NativeTokenHandler is IHandler, ERCHandlerHelpers, DepositDataHelper {
         optionalGas                        uint256 bytes (64 + len(destinationRecipientAddress)) - (96 + len(destinationRecipientAddress))
         optionalMessage             length uint256 bytes (96 + len(destinationRecipientAddress)) - (128 + len(destinationRecipientAddress))
         optionalMessage                    bytes   bytes (160 + len(destinationRecipientAddress)) - END
-        @return deposit amount internal representation.
+        @return 32-length byte array with internal bridge amount OR empty byte array if conversion is not needed.
      */
     function deposit(
         bytes32 resourceID,
@@ -66,7 +67,7 @@ contract NativeTokenHandler is IHandler, ERCHandlerHelpers, DepositDataHelper {
 
         address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
 
-        return abi.encodePacked(convertToInternalBalance(tokenAddress, amount));
+        return convertToInternalBalance(tokenAddress, amount);
     }
 
     /**
@@ -123,6 +124,7 @@ contract NativeTokenHandler is IHandler, ERCHandlerHelpers, DepositDataHelper {
         if (address(this).balance <= amount) revert InsufficientBalance();
         (, recipient, amount) = abi.decode(data, (address, address, uint));
 
+        recipient.mustNotBeZero();
         (bool success, ) = address(recipient).call{value: amount}("");
         if(!success) revert FailedFundsTransfer();
         emit Withdrawal(recipient, amount);

contracts/handlers/XC20Handler.sol

@@ -12,7 +12,9 @@ import "../XC20Safe.sol";
     @notice This contract is intended to be used with the Bridge contract.
  */
 contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
-        /**
+    using SanityChecks for *;
+
+    /**
         @param bridgeAddress Contract address of previously deployed Bridge.
      */
     constructor(
@@ -31,7 +33,7 @@ contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
         destinationRecipientAddress                 bytes       bytes  64 - END
         @dev Depending if the corresponding {tokenAddress} for the parsed {resourceID} is
         marked true in {_tokenContractAddressToTokenProperties[tokenAddress].isBurnable}, deposited tokens will be burned, if not, they will be locked.
-        @return an empty data.
+        @return 32-length byte array with internal bridge amount OR empty byte array if conversion is not needed.
      */
     function deposit(
         bytes32 resourceID,
@@ -50,7 +52,7 @@ contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
             lockERC20(tokenAddress, depositor, address(this), amount);
         }
 
-        return abi.encodePacked(convertToInternalBalance(tokenAddress, amount));
+        return convertToInternalBalance(tokenAddress, amount);
     }
 
     /**
@@ -70,7 +72,8 @@ contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
         bytes  memory destinationRecipientAddress;
 
         (amount, lenDestinationRecipientAddress) = abi.decode(data, (uint, uint));
-        destinationRecipientAddress = bytes(data[64:64 + lenDestinationRecipientAddress]);
+        lenDestinationRecipientAddress.mustBe(20);
+        destinationRecipientAddress = bytes(data[64:84]);
 
         bytes20 recipientAddress;
         address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
@@ -104,6 +107,7 @@ contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
 
         (tokenAddress, recipient, amount) = abi.decode(data, (address, address, uint));
 
+        recipient.mustNotBeZero();
         releaseERC20(tokenAddress, recipient, amount);
     }
 
@@ -116,6 +120,7 @@ contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
         @param args Additional data to be passed to specified handler.
      */
     function setResource(bytes32 resourceID, address contractAddress, bytes calldata args) external onlyBridge {
+        contractAddress.mustNotBeZero();
         _setResource(resourceID, contractAddress);
 
         uint8 externalTokenDecimals = uint8(bytes1(args));

contracts/handlers/fee/BasicFeeHandler.sol

@@ -4,6 +4,7 @@ pragma solidity 0.8.11;
 
 import "../../interfaces/IFeeHandler.sol";
 import "../../utils/AccessControl.sol";
+import "../../utils/SanityChecks.sol";
 import "../FeeHandlerRouter.sol";
 
 /**
@@ -12,6 +13,8 @@ import "../FeeHandlerRouter.sol";
     @notice This contract is intended to be used with the Bridge contract.
  */
 contract BasicFeeHandler is IFeeHandler, AccessControl {
+    using SanityChecks for *;
+
     address public immutable _bridgeAddress;
     address public immutable _feeHandlerRouterAddress;
     mapping (uint8 => mapping(bytes32 => uint256)) public _domainResourceIDToFee;
@@ -121,6 +124,7 @@ contract BasicFeeHandler is IFeeHandler, AccessControl {
     function transferFee(address payable[] calldata addrs, uint[] calldata amounts) external onlyAdmin {
         require(addrs.length == amounts.length, "addrs[], amounts[]: diff length");
         for (uint256 i = 0; i < addrs.length; i++) {
+            addrs[i].mustNotBeZero();
             (bool success,) = addrs[i].call{value: amounts[i]}("");
             require(success, "Fee ether transfer failed");
             emit FeeDistributed(address(0), addrs[i], amounts[i]);

contracts/handlers/fee/PercentageERC20FeeHandler.sol

@@ -5,6 +5,7 @@ pragma solidity 0.8.11;
 import "../../interfaces/IBridge.sol";
 import "../../interfaces/IERCHandler.sol";
 import "../../ERC20Safe.sol";
+import "../../utils/SanityChecks.sol";
 import { BasicFeeHandler } from "./BasicFeeHandler.sol";
 
 /**
@@ -13,6 +14,8 @@ import { BasicFeeHandler } from "./BasicFeeHandler.sol";
     @notice This contract is intended to be used with the Bridge contract.
  */
 contract PercentageERC20FeeHandler is BasicFeeHandler, ERC20Safe {
+    using SanityChecks for *;
+
     uint32 public constant HUNDRED_PERCENT = 1e8;
 
     /**
@@ -133,6 +136,7 @@ contract PercentageERC20FeeHandler is BasicFeeHandler, ERC20Safe {
         address tokenHandler = IBridge(_bridgeAddress)._resourceIDToHandlerAddress(resourceID);
         address tokenAddress = IERCHandler(tokenHandler)._resourceIDToTokenContractAddress(resourceID);
         for (uint256 i = 0; i < addrs.length; i++) {
+            addrs[i].mustNotBeZero();
             releaseERC20(tokenAddress, addrs[i], amounts[i]);
             emit FeeDistributed(tokenAddress, addrs[i], amounts[i]);
         }