IS-1120: prevalidate a tx more strictly (#525)

* Pre-validate on entered request parameters
* Malformed number string
* Not allow uppercase character
* Malformed address
* v3 protocol only
* Required fields check
* testcase

iconservice/base/type_converter_templates.py

@@ -107,6 +107,7 @@ class ConstantKeys:
     FROM = "from"
     TO = "to"
     VALUE = "value"
+    NID = "nid"
     STEP_LIMIT = "stepLimit"
     FEE = "fee"
     NONCE = "nonce"

iconservice/icon_inner_service.py

@@ -409,8 +409,9 @@ async def validate_transaction(self, request: dict):
 
     def _validate_transaction(self, request: dict):
         try:
+            Logger.info(tag=_TAG, msg=f'validate_transaction Request: {request}')
             converted_request = TypeConverter.convert(request, ParamType.VALIDATE_TRANSACTION)
-            self._icon_service_engine.validate_transaction(converted_request)
+            self._icon_service_engine.validate_transaction(converted_request, request)
             response = MakeResponse.make_response(ExceptionCode.OK)
         except FatalException as e:
             self._log_exception(e, _TAG)

iconservice/icon_service_engine.py

@@ -1064,14 +1064,15 @@ def query(self, method: str, params: dict) -> Any:
         ret = self._call(context, method, params)
         return ret
 
-    def validate_transaction(self, request: dict) -> None:
+    def validate_transaction(self, request: dict, origin_request: dict) -> None:
         """Validate JSON-RPC transaction request
         before putting it into transaction pool
 
         JSON Schema validator checks basic JSON-RPC request syntax
         on JSON-RPC Server
         IconPreValidator focuses on business logic and semantic problems
 
+        :param origin_request: JSON_RPC Original request for more strict validate
         :param request: JSON-RPC request
             values in request have already been converted to original format
             in IconInnerService
@@ -1089,6 +1090,8 @@ def validate_transaction(self, request: dict) -> None:
         context = self._context_factory.create(IconScoreContextType.QUERY, self._get_last_block())
         context.set_step_counter()
 
+        origin_params = origin_request['params']
+
         try:
             self._push_context(context)
 
@@ -1101,7 +1104,7 @@ def validate_transaction(self, request: dict) -> None:
                 data = params['data']
                 input_size = get_input_data_size(context.revision, data)
                 minimum_step += input_size * context.inv_container.step_costs.get(StepType.INPUT, 0)
-
+            self._icon_pre_validator.origin_request_execute(origin_params, context.revision)
             self._icon_pre_validator.execute(context, params, step_price, minimum_step)
 
             if to.is_contract:

iconservice/iconscore/icon_pre_validator.py

@@ -19,8 +19,10 @@
 from iconcommons.logger import Logger
 
 from .icon_score_step import get_input_data_size
-from ..base.address import Address, AddressPrefix, SYSTEM_SCORE_ADDRESS, generate_score_address
+
+from ..base.address import Address, AddressPrefix, SYSTEM_SCORE_ADDRESS, generate_score_address, is_icon_address_valid
 from ..base.exception import InvalidRequestException, InvalidParamsException, OutOfBalanceException
+from ..base.type_converter_templates import ConstantKeys
 from ..icon_constant import (
     FIXED_FEE, MAX_DATA_SIZE, DEFAULT_BYTE_SIZE,
     DATA_BYTE_ORDER, Revision, DeployState, DataType
@@ -34,6 +36,43 @@
 
 
 TAG = "PV"
+REQUEST_PARAMS = (
+    ConstantKeys.VERSION,
+    ConstantKeys.STEP_LIMIT,
+    ConstantKeys.NID,
+    ConstantKeys.TIMESTAMP,
+    ConstantKeys.VALUE,
+    ConstantKeys.NONCE,
+    ConstantKeys.FROM,
+    ConstantKeys.TO,
+    ConstantKeys.SIGNATURE,
+    ConstantKeys.DATA_TYPE,
+    ConstantKeys.DATA,
+)
+
+REQUIRED_PARAMS = (
+    ConstantKeys.VERSION,
+    ConstantKeys.STEP_LIMIT,
+    ConstantKeys.NID,
+    ConstantKeys.TIMESTAMP,
+    ConstantKeys.FROM,
+    ConstantKeys.TO,
+    ConstantKeys.SIGNATURE
+)
+
+INT_PARAMS = (
+    ConstantKeys.VERSION,
+    ConstantKeys.STEP_LIMIT,
+    ConstantKeys.NID,
+    ConstantKeys.TIMESTAMP,
+    ConstantKeys.VALUE,
+    ConstantKeys.NONCE
+)
+
+ADDR_PARAMS = (
+    ConstantKeys.FROM,
+    ConstantKeys.TO
+)
 
 
 class IconPreValidator:
@@ -47,6 +86,69 @@ def __init__(self) -> None:
         """
         pass
 
+    def origin_request_execute(self, params: dict, revision: int):
+        if revision < Revision.IMPROVED_PRE_VALIDATOR.value:
+            return
+        self.origin_pre_validate_version(params)
+        self.origin_pre_validate_params(params)
+        self.origin_validate_fields(params)
+
+    def origin_validate_fields(self, params: dict):
+        for param, value in params.items():
+            self.origin_validate_param(param)
+            self.origin_validate_value(param, value)
+
+    @classmethod
+    def origin_pre_validate_version(cls, params: dict):
+        version: str = params.get(ConstantKeys.VERSION, None)
+        if version != '0x3':
+            raise InvalidRequestException(f'Invalid message version, got {version}')
+
+    @classmethod
+    def origin_pre_validate_params(cls, params: dict):
+        if len(params) > len(REQUEST_PARAMS):
+            raise InvalidRequestException('Unexpected Parameters')
+
+        required_results = [
+            required_key
+            for required_key
+            in REQUIRED_PARAMS
+            if required_key not in params
+        ]
+
+        if required_results:
+            raise InvalidRequestException(
+                f'Not included required parameters, missing parameters {required_results}'
+            )
+
+    @classmethod
+    def origin_validate_param(cls, param: str):
+        if param not in REQUEST_PARAMS:
+            raise InvalidParamsException(f'Unexpected Parameters, got {param}')
+
+    @classmethod
+    def origin_validate_value(cls, param: str, value: str):
+        if param in INT_PARAMS:
+            if not cls.is_integer_type(value):
+                raise InvalidRequestException(f'Unexpected INT Type, got {value}')
+        elif param in ADDR_PARAMS:
+            if not cls.is_address_type(value):
+                raise InvalidRequestException(f'Unexpected Address Type, got {value}')
+
+    @classmethod
+    def is_integer_type(cls, value: str) -> bool:
+        try:
+            if value.startswith('0x'):
+                return value == hex(int(value, 16))
+        except ValueError:
+            return False
+        else:
+            return False
+
+    @classmethod
+    def is_address_type(cls, value: str) -> bool:
+        return is_icon_address_valid(value)
+
     def execute(self, context: 'IconScoreContext', params: dict, step_price: int, minimum_step: int):
         """Validate a transaction on icx_sendTransaction
         If failed to validate a tx, raise an exception

tests/integrate_test/iiss/decentralized/test_call.py

@@ -78,15 +78,15 @@ def test_validate_data_type(self):
             value=value,
             disable_pre_validate=True
         )
-
-        self.icon_service_engine.validate_transaction(tx)
+        origin_params = {'params': self.make_origin_params(tx['params'])}
+        self.icon_service_engine.validate_transaction(tx, origin_params)
 
         invalid_data_types = ("abc", 1, 1.1, b"call", b"deploy", b"deposit", b"message")
         for data_type in invalid_data_types:
             tx["params"]["dataType"] = data_type
 
             with self.assertRaises(InvalidParamsException):
-                self.icon_service_engine.validate_transaction(tx)
+                self.icon_service_engine.validate_transaction(tx, origin_params)
 
         for data_type in DataType._TYPES:
             params = tx["params"]
@@ -100,7 +100,7 @@ def test_validate_data_type(self):
             else:
                 continue
 
-            self.icon_service_engine.validate_transaction(tx)
+            self.icon_service_engine.validate_transaction(tx, origin_params)
 
     def test_invalid_score_call_failure_on_invoke(self):
         self.set_revision(Revision.IMPROVED_PRE_VALIDATOR.value)

tests/integrate_test/test_integrate_base.py

@@ -342,6 +342,33 @@ def _write_precommit_state_in_leader(self, block_height: int, old_block_hash: by
         assert block_height == self._block_height
         self._prev_block_hash = new_block_hash
 
+    def make_origin_params(self, params: dict):
+        origin_items = [
+            ('version', int, hex),
+            ('from', Address, str),
+            ('to', Address, str),
+            ('value', int, hex),
+            ('nonce', int, hex),
+            ('stepLimit', int, hex),
+            ('timestamp', int, hex),
+            ('nid', int, hex),
+            ('signature', str, str)
+        ]
+        origin_params = params.copy()
+
+        if "txHash" in origin_params:
+            del origin_params["txHash"]
+
+        for key, value_type, origin_type in origin_items:
+            value = origin_params.get(key, None)
+            if value is None and key == "nid":
+                origin_params[key] = hex(2)
+
+            if isinstance(value, value_type):
+                origin_params[key] = origin_type(value)
+
+        return origin_params
+
     def rollback(self, block_height: int = -1, block_hash: Optional[bytes] = None):
         """Rollback the current state to the old one indicated by a given block
 
@@ -473,6 +500,7 @@ def create_deploy_score_tx(self,
             "data": deploy_data
         }
 
+        origin_params = {'params': self.make_origin_params(request_params)}
         method = 'icx_sendTransaction'
         # Insert txHash into request params
         request_params['txHash'] = create_tx_hash()
@@ -482,7 +510,7 @@ def create_deploy_score_tx(self,
         }
 
         if pre_validation_enabled:
-            self.icon_service_engine.validate_transaction(tx)
+            self.icon_service_engine.validate_transaction(tx, origin_params)
 
         return tx
 
@@ -518,6 +546,9 @@ def create_score_call_tx(self,
             }
         }
 
+        origin_params = {
+            'params': self.make_origin_params(request_params)
+        }
         method = 'icx_sendTransaction'
         # Insert txHash into request params
         request_params['txHash'] = create_tx_hash()
@@ -527,7 +558,7 @@ def create_score_call_tx(self,
         }
 
         if pre_validation_enabled:
-            self.icon_service_engine.validate_transaction(tx)
+            self.icon_service_engine.validate_transaction(tx, origin_params)
 
         return tx
 
@@ -560,6 +591,9 @@ def create_transfer_icx_tx(self,
         else:
             request_params["version"] = self._version
 
+        origin_params = {
+            'params': self.make_origin_params(request_params)
+        }
         method = 'icx_sendTransaction'
         # Insert txHash into request params
         request_params['txHash'] = create_tx_hash()
@@ -569,7 +603,7 @@ def create_transfer_icx_tx(self,
         }
 
         if not disable_pre_validate:
-            self.icon_service_engine.validate_transaction(tx)
+            self.icon_service_engine.validate_transaction(tx, origin_params)
         return tx
 
     def create_message_tx(self,
@@ -598,6 +632,10 @@ def create_message_tx(self,
             "data": '0x' + data.hex(),
         }
 
+        origin_params = {
+            'params': self.make_origin_params(request_params)
+        }
+
         method = 'icx_sendTransaction'
         # Inserts txHash into request params
         request_params['txHash'] = create_tx_hash()
@@ -607,7 +645,7 @@ def create_message_tx(self,
         }
 
         if pre_validation_enabled:
-            self.icon_service_engine.validate_transaction(tx)
+            self.icon_service_engine.validate_transaction(tx, origin_params)
         return tx
 
     def create_deposit_tx(self,
@@ -639,7 +677,9 @@ def create_deposit_tx(self,
                 "action": action,
             }
         }
-
+        origin_params = {
+            'params': self.make_origin_params(request_params)
+        }
         for k, v in params.items():
             request_params["data"][k] = v
 
@@ -652,7 +692,7 @@ def create_deposit_tx(self,
         }
 
         if pre_validation_enabled:
-            self.icon_service_engine.validate_transaction(tx)
+            self.icon_service_engine.validate_transaction(tx, origin_params)
 
         return tx