Fix 1271 contract signatures

- Closes #2535 and #2526

Author: Uxio0

safe_transaction_service/safe_messages/serializers.py

@@ -9,14 +9,17 @@
 from rest_framework import serializers
 from rest_framework.exceptions import ValidationError
 from safe_eth.eth import get_auto_ethereum_client
-from safe_eth.eth.eip712 import eip712_encode_hash
+from safe_eth.eth.eip712 import eip712_encode
 from safe_eth.safe.safe_signature import SafeSignature, SafeSignatureType
 from safe_eth.util.util import to_0x_hex_str
 
 from safe_transaction_service.utils.serializers import get_safe_owners
 
 from .models import SIGNATURE_LENGTH, SafeMessage, SafeMessageConfirmation
-from .utils import get_hash_for_message, get_safe_message_hash_for_message
+from .utils import (
+    get_message_encoded,
+    get_safe_message_hash_and_preimage_for_message,
+)
 
 
 # Request serializers
@@ -93,7 +96,7 @@ def validate_message(self, value: Union[str, dict[str, Any]]):
 
         if isinstance(value, dict):
             try:
-                eip712_encode_hash(value)
+                eip712_encode(value)
                 return value
             except ValueError as exc:
                 raise ValidationError(
@@ -105,12 +108,15 @@ def validate_message(self, value: Union[str, dict[str, Any]]):
     def validate(self, attrs):
         attrs = super().validate(attrs)
         safe_address = self.context["safe_address"]
+        attrs["safe"] = safe_address
         message = attrs["message"]
         signature = attrs["signature"]
-        attrs["safe"] = safe_address
-        message_hash = get_hash_for_message(message)
-        safe_message_hash = get_safe_message_hash_for_message(
-            safe_address, message_hash
+        # Encode EIP-191 or EIP-712 original message as bytes
+        message_encoded = get_message_encoded(message)
+        safe_message_hash, safe_message_preimage = (
+            get_safe_message_hash_and_preimage_for_message(
+                safe_address, message_encoded
+            )
         )
         attrs["message_hash"] = safe_message_hash
 
@@ -119,8 +125,11 @@ def validate(self, attrs):
                 f"Message with hash {to_0x_hex_str(safe_message_hash)} for safe {safe_address} already exists in DB"
             )
 
+        # Preimage is encoded for the Safe. But if an EIP-1271 signature is used, owner's Safe will be called
+        # the preimage will be encoded again for the owner Safe. That's what needs to be signed by the user
+        # So original data -> EIP-191 or EIP-712 encoded -> Safe encoded data -> Owner encoded data
         safe_signatures = SafeSignature.parse_signature(
-            signature, safe_message_hash, safe_hash_preimage=message_hash
+            signature, safe_message_hash, safe_hash_preimage=safe_message_preimage
         )
         owner, signature_type = self.get_valid_owner_from_signatures(
             safe_signatures, safe_address, None
@@ -158,11 +167,16 @@ def validate(self, attrs):
         attrs["safe_message"] = safe_message
         signature: HexStr = attrs["signature"]
         safe_address = safe_message.safe
-        message_hash = get_hash_for_message(safe_message.message)
-        safe_message_hash = safe_message.message_hash
+        message_encoded = get_message_encoded(safe_message.message)
+        safe_message_hash, safe_message_preimage = (
+            get_safe_message_hash_and_preimage_for_message(
+                safe_address, message_encoded
+            )
+        )
+        assert to_0x_hex_str(safe_message_hash) == safe_message.message_hash
 
         safe_signatures = SafeSignature.parse_signature(
-            signature, safe_message_hash, safe_hash_preimage=message_hash
+            signature, safe_message_hash, safe_hash_preimage=safe_message_preimage
         )
         owner, signature_type = self.get_valid_owner_from_signatures(
             safe_signatures, safe_address, safe_message

safe_transaction_service/safe_messages/tests/factories.py

@@ -5,7 +5,7 @@
 from safe_eth.util.util import to_0x_hex_str
 
 from ..models import SafeMessage, SafeMessageConfirmation
-from ..utils import get_hash_for_message, get_safe_message_hash_for_message
+from ..utils import get_message_encoded, get_safe_message_hash_and_preimage_for_message
 
 
 class SafeMessageFactory(DjangoModelFactory):
@@ -21,9 +21,9 @@ class Meta:
     @factory.lazy_attribute
     def message_hash(self) -> str:
         return to_0x_hex_str(
-            get_safe_message_hash_for_message(
-                self.safe, get_hash_for_message(self.message)
-            )
+            get_safe_message_hash_and_preimage_for_message(
+                self.safe, get_message_encoded(self.message)
+            )[0]
         )
 
 

safe_transaction_service/safe_messages/tests/test_models.py

@@ -8,7 +8,7 @@
 from safe_eth.safe.tests.safe_test_case import SafeTestCaseMixin
 from safe_eth.util.util import to_0x_hex_str
 
-from ..utils import get_hash_for_message, get_safe_message_hash_for_message
+from ..utils import get_message_encoded, get_safe_message_hash_and_preimage_for_message
 from .factories import SafeMessageConfirmationFactory, SafeMessageFactory
 from .mocks import get_eip712_payload_mock
 
@@ -22,15 +22,15 @@ def test_str(self):
         for input, expected in [
             (
                 "TestMessage",
-                "Safe Message 0xb04a24aa07a51d1d8c3913e9493b3b1f88ed6a8a75430a9a8eda3ed3ce1897bc - TestMessage",
+                "Safe Message 0x95a89792bad17115e101b4d8fbdcd517dd13d085e815287d7fe791078182bf9e - TestMessage",
             ),
             (
                 "TestMessageVeryLong",
-                "Safe Message 0xe3db816540ce371e2703b8ec59bdd6fec32e0c6078f2e204a205fd6d81564f28 - TestMessageVery...",
+                "Safe Message 0xb7783fde9060e75b61298c14cbc2a987f0e0800d1bb08b4a2b24ce3544b9b144 - TestMessageVery...",
             ),
             (
                 get_eip712_payload_mock(),
-                "Safe Message 0xbabb22f5c02a24db447b8f0136d6e26bb58cd6d068ebe8ab25c2221cfdf53e18 - {'types': {'EIP...",
+                "Safe Message 0x632db20317ce8e467d17e941d209fc1173ff1cad83b0c072c008385a566ddd80 - {'types': {'EIP...",
             ),
         ]:
             with self.subTest(input=input):
@@ -61,9 +61,9 @@ def test_factory(self):
         self.assertEqual(
             message_hash,
             to_0x_hex_str(
-                get_safe_message_hash_for_message(
-                    safe_message.safe, get_hash_for_message(message)
-                )
+                get_safe_message_hash_and_preimage_for_message(
+                    safe_message.safe, get_message_encoded(message)
+                )[0]
             ),
         )
         recovered_owner = Account._recover_hash(

safe_transaction_service/safe_messages/tests/test_views.py

@@ -7,12 +7,11 @@
 
 import eth_abi
 from eth_account import Account
-from eth_account.messages import defunct_hash_message
 from hexbytes import HexBytes
 from rest_framework import status
 from rest_framework.exceptions import ErrorDetail
 from rest_framework.test import APITestCase
-from safe_eth.eth.eip712 import eip712_encode_hash
+from safe_eth.eth.eip712 import eip712_encode
 from safe_eth.safe.safe_signature import SafeSignatureEOA
 from safe_eth.safe.signatures import signature_to_bytes
 from safe_eth.safe.tests.safe_test_case import SafeTestCaseMixin
@@ -28,6 +27,7 @@
 )
 from safe_transaction_service.utils.utils import datetime_to_str
 
+from ..utils import encode_eip191_message, encode_eip712_message
 from .mocks import get_eip712_payload_mock
 
 logger = logging.getLogger(__name__)
@@ -144,12 +144,13 @@ def test_safe_messages_create_view(self, get_owners_mock: MagicMock):
         safe_address = safe.address
         messages = ["Text to sign message", get_eip712_payload_mock()]
         description = "Testing EIP191 message signing"
-        message_hashes = [
-            defunct_hash_message(text=messages[0]),
-            eip712_encode_hash(messages[1]),
+        messages_encoded = [
+            encode_eip191_message(messages[0]),
+            encode_eip712_message(messages[1]),
         ]
         safe_message_hashes = [
-            safe.get_message_hash(message_hash) for message_hash in message_hashes
+            safe.get_message_hash(message_encoded)
+            for message_encoded in messages_encoded
         ]
         signatures = [
             to_0x_hex_str(account.unsafe_sign_hash(safe_message_hash)["signature"])
@@ -158,15 +159,14 @@ def test_safe_messages_create_view(self, get_owners_mock: MagicMock):
 
         sub_tests = ["create_eip191", "create_eip712"]
 
-        for sub_test, message, message_hash, safe_message_hash, signature in zip(
-            sub_tests, messages, message_hashes, safe_message_hashes, signatures
+        for sub_test, message, safe_message_hash, signature in zip(
+            sub_tests, messages, safe_message_hashes, signatures
         ):
             SafeMessage.objects.all().delete()
             get_owners_mock.return_value = []
             with self.subTest(
                 sub_test,
                 message=message,
-                message_hash=message_hash,
                 safe_message_hash=safe_message_hash,
                 signature=signature,
             ):
@@ -289,8 +289,13 @@ def _test_safe_messages_create_using_1271_signature_view(self, safe_deployment_f
         safe_address = safe.address
         message = get_eip712_payload_mock()
         description = "Testing EIP712 message signing"
-        message_hash = eip712_encode_hash(message)
-        safe_owner_message_hash = safe_owner.get_message_hash(message_hash)
+        message_encoded = b"".join(eip712_encode(message))
+        safe_message_hash, safe_message_preimage = safe.get_message_hash_and_preimage(
+            message_encoded
+        )
+        safe_owner_message_hash, _ = safe_owner.get_message_hash_and_preimage(
+            safe_message_preimage
+        )
         safe_owner_signature = account.unsafe_sign_hash(safe_owner_message_hash)[
             "signature"
         ]
@@ -314,8 +319,12 @@ def _test_safe_messages_create_using_1271_signature_view(self, safe_deployment_f
             data=data,
         )
         self.assertEqual(response.status_code, status.HTTP_201_CREATED)
-        self.assertEqual(SafeMessage.objects.count(), 1)
-        self.assertEqual(SafeMessageConfirmation.objects.count(), 1)
+        self.assertEqual(
+            SafeMessage.objects.get().message_hash, to_0x_hex_str(safe_message_hash)
+        )
+        self.assertEqual(
+            SafeMessageConfirmation.objects.get().owner, safe_owner.address
+        )
 
     def test_safe_messages_create_using_1271_signature_v1_3_0_view(self):
         return self._test_safe_messages_create_using_1271_signature_view(

safe_transaction_service/safe_messages/utils.py

@@ -1,22 +1,36 @@
 from typing import Any
 
-from eth_account.messages import defunct_hash_message
+from eth_account.messages import encode_defunct
 from eth_typing import ChecksumAddress, Hash32
 from safe_eth.eth import get_auto_ethereum_client
-from safe_eth.eth.eip712 import eip712_encode_hash
+from safe_eth.eth.eip712 import eip712_encode
 from safe_eth.safe import Safe
 
 
-def get_hash_for_message(message: str | dict[str, Any]) -> Hash32:
+def encode_eip191_message(message: str) -> bytes:
+    signable_message = encode_defunct(text=message)
     return (
-        defunct_hash_message(text=message)
+        b"\x19"
+        + signable_message.version
+        + signable_message.header
+        + signable_message.body
+    )
+
+
+def encode_eip712_message(message: dict[str, Any]) -> bytes:
+    return b"".join(eip712_encode(message))
+
+
+def get_message_encoded(message: str | dict[str, Any]) -> bytes:
+    return (
+        encode_eip191_message(message)
         if isinstance(message, str)
-        else eip712_encode_hash(message)
+        else encode_eip712_message(message)
     )
 
 
-def get_safe_message_hash_for_message(
-    safe_address: ChecksumAddress, message_hash: Hash32
-) -> Hash32:
+def get_safe_message_hash_and_preimage_for_message(
+    safe_address: ChecksumAddress, message: bytes
+) -> tuple[Hash32, bytes]:
     safe = Safe(safe_address, get_auto_ethereum_client())
-    return safe.get_message_hash(message_hash)
+    return safe.get_message_hash_and_preimage(message)