Skip to content

Commit

Permalink
fix: API uses .signHash raw hash signing (dangerous)
Browse files Browse the repository at this point in the history
  • Loading branch information
fubuloubu committed Mar 12, 2024
1 parent bb2c2f4 commit 1f62100
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 12 deletions.
40 changes: 32 additions & 8 deletions ape_safe/client/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
from typing import Dict, Iterator, List, Optional, Union, cast

from ape.api import AccountAPI
from ape.logging import logger
from ape.types import AddressType, HexBytes, MessageSignature
from ape_accounts import KeyfileAccount
from eip712.common import SafeTxV1, SafeTxV2
from eth_account import Account as EthAccount

from ape_safe.client.base import BaseSafeClient
from ape_safe.client.mock import MockSafeClient
Expand Down Expand Up @@ -198,26 +201,47 @@ def get_delegates(self) -> Dict[AddressType, List[AddressType]]:
return delegates

def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI):
msg = self.create_delegate_message(delegate)
if not (sig := delegator.sign_message(msg)):
raise ActionNotPerformedError("Must sign request to add delegate.")
# TODO: Replace this by adding raw hash signing into supported account plugins
if not isinstance(delegator, KeyfileAccount):
raise ActionNotPerformedError("Need access to private key for this method.")

logger.warning("Need to unlock account to add a delegate.")
delegator.unlock() # NOTE: Ensures we have the key handy

msg_hash = self.create_delegate_message(delegate)
# NOTE: This is required as Safe API uses an antiquated .signHash method
sig = EthAccount.signHash(
msg_hash,
delegator._KeyfileAccount__cached_key, # type: ignore[attr-defined]
)

payload = {
"safe": self.address,
"delegate": delegate,
"delegator": delegator.address,
"label": label,
"signature": sig.encode_rsv().hex(),
"signature": sig.signature.hex(),
}
self._post("delegates", json=payload)

def remove_delegate(self, delegate: AddressType, delegator: AccountAPI):
msg = self.create_delegate_message(delegate)
if not (sig := delegator.sign_message(msg)):
raise ActionNotPerformedError("Must sign request to remove delegate.")
# TODO: Replace this by adding raw hash signing into supported account plugins
if not isinstance(delegator, KeyfileAccount):
raise ActionNotPerformedError("Need access to private key for this method.")

logger.warning("Need to unlock account to add a delegate.")
delegator.unlock() # NOTE: Ensures we have the key handy

msg_hash = self.create_delegate_message(delegate)
# NOTE: This is required as Safe API uses an antiquated .signHash method
sig = EthAccount.signHash(
msg_hash,
delegator._KeyfileAccount__cached_key, # type: ignore[attr-defined]
)

payload = {
"signature": sig.encode_rsv().hex(),
"delegator": delegator.address,
"signature": sig.signature.hex(),
}
self._delete(f"delegates/{delegate}", json=payload)

Expand Down
7 changes: 3 additions & 4 deletions ape_safe/client/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
import requests
import urllib3
from ape.api import AccountAPI
from ape.types import AddressType, MessageSignature, SignableMessage
from eth_account.messages import encode_defunct as encode_eip191_signable_message
from ape.types import AddressType, MessageSignature
from eth_utils import keccak
from requests import Response
from requests.adapters import HTTPAdapter
Expand Down Expand Up @@ -114,11 +113,11 @@ def get_transactions(

yield txn

def create_delegate_message(self, delegate: AddressType) -> SignableMessage:
def create_delegate_message(self, delegate: AddressType) -> bytes:
# NOTE: referencing https://github.com/safe-global/safe-eth-py/blob/
# a0a5771622f143ee6301cfc381c5ed50832ff482/gnosis/safe/api/transaction_service_api.py#L34
totp = int(time.time()) // 3600
return encode_eip191_signable_message(keccak(text=(delegate + str(totp))))
return keccak(text=(delegate + str(totp)))

@abstractmethod
def get_delegates(self) -> Dict[AddressType, List[AddressType]]: ...
Expand Down

0 comments on commit 1f62100

Please sign in to comment.