Validate signature's s-value in transaction validity (#354)

We validate that a transaction signature's s-value is not too high. How
high the s-value can be depends on the fork (Homestead changed the
value). We make use of `EVM.Configuration` to ensure that all forks
provide the correct s-value.

apps/blockchain/lib/blockchain/account.ex

@@ -123,7 +123,7 @@ defmodule Blockchain.Account do
       ...> |> Blockchain.Account.get_account(<<0x01::160>>)
       nil
   """
-  @spec get_account(EVM.state(), EVM.address()) :: t | nil
+  @spec get_account(Trie.t(), EVM.address()) :: t | nil
   def get_account(state, address) do
     address = if is_binary(address), do: address, else: :binary.encode_unsigned(address)
     trie = Trie.get(state, Keccak.kec(address))

apps/blockchain/lib/blockchain/transaction/signature.ex

@@ -26,6 +26,9 @@ defmodule Blockchain.Transaction.Signature do
   @base_recovery_id 27
   @base_recovery_id_eip_155 35
 
+  def secp256k1n, do: @secp256k1n
+  def secp256k1n_2, do: @secp256k1n_2
+
   @doc """
   Given a private key, returns a public key.
 
@@ -153,41 +156,44 @@ defmodule Blockchain.Transaction.Signature do
 
   ## Examples
 
-    iex> Blockchain.Transaction.Signature.is_signature_valid?(true, 1, 1, 27)
+    iex> Blockchain.Transaction.Signature.is_signature_valid?(1, 1, 27, max_s: :secp256k1n)
     true
 
-    iex> Blockchain.Transaction.Signature.is_signature_valid?(true, 1, 1, 20) # invalid v
+    iex> Blockchain.Transaction.Signature.is_signature_valid?(1, 1, 20, max_s: :secp256k1n) # invalid v
     false
 
     iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337
-    iex> Blockchain.Transaction.Signature.is_signature_valid?(false, secp256k1n - 1, 1, 28) # r okay
+    iex> Blockchain.Transaction.Signature.is_signature_valid?(secp256k1n - 1, 1, 28, max_s: :secp256k1n) # r okay
     true
 
     iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337
-    iex> Blockchain.Transaction.Signature.is_signature_valid?(false, secp256k1n + 1, 1, 28) # r too high
+    iex> Blockchain.Transaction.Signature.is_signature_valid?(secp256k1n + 1, 1, 28, max_s: :secp256k1n) # r too high
     false
 
     iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337
-    iex> Blockchain.Transaction.Signature.is_signature_valid?(false, 1, secp256k1n + 1, 28) # s too high for non-homestead
+    iex> Blockchain.Transaction.Signature.is_signature_valid?(1, secp256k1n + 1, 28, max_s: :secp256k1n) # s too high for non-homestead
     false
 
     iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337
-    iex> Blockchain.Transaction.Signature.is_signature_valid?(false, 1, secp256k1n - 1, 28) # s okay for non-homestead
+    iex> Blockchain.Transaction.Signature.is_signature_valid?(1, secp256k1n - 1, 28, max_s: :secp256k1n) # s okay for non-homestead
     true
 
     iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337
-    iex> Blockchain.Transaction.Signature.is_signature_valid?(true, 1, secp256k1n - 1, 28) # s too high for homestead
+    iex> Blockchain.Transaction.Signature.is_signature_valid?(1, secp256k1n - 1, 28, max_s: :secp256k1n_2) # s too high for homestead
     false
 
     iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337
     iex> secp256k1n_2 = round(:math.floor(secp256k1n / 2))
-    iex> Blockchain.Transaction.Signature.is_signature_valid?(true, secp256k1n_2 - 1, 1, 28) # s okay for homestead
+    iex> Blockchain.Transaction.Signature.is_signature_valid?(secp256k1n_2 - 1, 1, 28, max_s: :secp256k1n_2) # s okay for homestead
     true
   """
-  @spec is_signature_valid?(boolean(), integer(), integer(), integer()) :: boolean()
-  def is_signature_valid?(is_homestead, r, s, v) do
-    r > 0 and r < @secp256k1n and s > 0 and
-      if(is_homestead, do: s < @secp256k1n_2, else: s < @secp256k1n) and (v == 27 || v == 28)
+  @spec is_signature_valid?(integer(), integer(), integer(), max_s: atom()) :: boolean()
+  def is_signature_valid?(r, s, v, max_s: :secp256k1n) do
+    r > 0 and r < @secp256k1n and s > 0 and s < @secp256k1n and (v == 27 || v == 28)
+  end
+
+  def is_signature_valid?(r, s, v, max_s: :secp256k1n_2) do
+    r > 0 and r < @secp256k1n and s > 0 and s <= @secp256k1n_2 and (v == 27 || v == 28)
   end
 
   @doc """

apps/blockchain/lib/blockchain/transaction/validity.ex

@@ -6,31 +6,19 @@ defmodule Blockchain.Transaction.Validity do
 
   alias Blockchain.{Account, Transaction}
   alias Block.Header
+  alias MerklePatriciaTree.Trie
 
   @doc """
   Validates the validity of a transaction that is required to be
   true before we're willing to execute a transaction. This is
   specified in Section 6.2 of the Yellow Paper Eq.(65) and Eq.(66).
   """
-  @spec validate(EVM.state(), Transaction.t(), Block.Header.t(), EVM.Configuration.t()) ::
+  @spec validate(Trie.t(), Transaction.t(), Block.Header.t(), EVM.Configuration.t()) ::
           :valid | {:invalid, atom()}
   def validate(state, trx, block_header, config) do
-    result =
-      case Transaction.Signature.sender(trx) do
-        {:error, _reason} ->
-          {:invalid, :invalid_sender}
-
-        {:ok, sender_address} ->
-          case Account.get_account(state, sender_address) do
-            nil ->
-              {:invalid, :missing_account}
-
-            sender_account ->
-              {:ok, sender_account}
-          end
-      end
-
-    with {:ok, sender_account} <- result do
+    with :ok <- validate_signature(trx, config),
+         {:ok, sender_address} <- Transaction.Signature.sender(trx),
+         {:ok, sender_account} <- retrieve_account(state, sender_address) do
       errors =
         []
         |> check_sender_nonce(trx, sender_account)
@@ -42,6 +30,30 @@ defmodule Blockchain.Transaction.Validity do
     end
   end
 
+  @spec validate_signature(Transaction.t(), EVM.Configuration.t()) ::
+          :ok | {:invalid, :invalid_signature}
+  defp validate_signature(trx, config) do
+    max_s_value = EVM.Configuration.max_signature_s(config)
+
+    if Transaction.Signature.is_signature_valid?(trx.r, trx.s, trx.v, max_s: max_s_value) do
+      :ok
+    else
+      {:invalid, :invalid_sender}
+    end
+  end
+
+  @spec retrieve_account(Trie.t(), EVM.address()) ::
+          {:ok, Account.t()} | {:invalid, :missing_account}
+  defp retrieve_account(state, sender_address) do
+    case Account.get_account(state, sender_address) do
+      nil ->
+        {:invalid, :missing_account}
+
+      sender_account ->
+        {:ok, sender_account}
+    end
+  end
+
   @spec check_sender_nonce([atom()], Transaction.t(), Account.t()) :: [atom()]
   defp check_sender_nonce(errors, transaction, account) do
     if account.nonce != transaction.nonce do

apps/blockchain/test/blockchain/transaction/validity_test.exs

@@ -4,6 +4,32 @@ defmodule Blockchain.Transaction.ValidityTest do
   alias Blockchain.{Transaction, Account}
 
   describe "validate/3" do
+    test "invalidates transaction when signature's s-value is too high (after homestead fork)" do
+      trx = %Transaction{
+        data: <<>>,
+        gas_limit: 1_000,
+        gas_price: 1,
+        init: <<1>>,
+        nonce: 5,
+        to: <<>>,
+        value: 5,
+        r: 1,
+        s: Transaction.Signature.secp256k1n_2() + 1,
+        v: 27
+      }
+
+      result =
+        MerklePatriciaTree.Test.random_ets_db()
+        |> MerklePatriciaTree.Trie.new()
+        |> Validity.validate(
+          trx,
+          %Block.Header{},
+          EVM.Configuration.Homestead.new()
+        )
+
+      assert result == {:invalid, :invalid_sender}
+    end
+
     test "invalidates transaction when sender address is nil" do
       trx = %Transaction{
         data: <<>>,
@@ -138,7 +164,7 @@ defmodule Blockchain.Transaction.ValidityTest do
       assert result == {:invalid, [:over_gas_limit, :insufficient_balance]}
     end
 
-    test "invalidates account when tranaction gas limit exceeds header gas limit" do
+    test "invalidates account when transaction gas limit exceeds header gas limit" do
       private_key = <<1::256>>
       # based on simple private key
       sender =

apps/evm/lib/evm/configuration.ex

@@ -13,6 +13,10 @@ defprotocol EVM.Configuration do
   @spec fail_contract_creation_lack_of_gas?(t) :: boolean()
   def fail_contract_creation_lack_of_gas?(t)
 
+  # EIP2
+  @spec max_signature_s(t) :: atom()
+  def max_signature_s(t)
+
   # EIP7
   @spec has_delegate_call?(t) :: boolean()
   def has_delegate_call?(t)

apps/evm/lib/evm/configuration/eip150.ex

@@ -23,6 +23,9 @@ defimpl EVM.Configuration, for: EVM.Configuration.EIP150 do
   @spec has_delegate_call?(Configuration.t()) :: boolean()
   def has_delegate_call?(config), do: config.fallback_config.has_delegate_call
 
+  @spec max_signature_s(Configuration.t()) :: atom()
+  def max_signature_s(config), do: Configuration.max_signature_s(config.fallback_config)
+
   @spec fail_contract_creation_lack_of_gas?(Configuration.t()) :: boolean()
   def fail_contract_creation_lack_of_gas?(config),
     do: config.fallback_config.fail_contract_creation

apps/evm/lib/evm/configuration/eip158.ex

@@ -19,6 +19,9 @@ defimpl EVM.Configuration, for: EVM.Configuration.EIP158 do
   @spec has_delegate_call?(Configuration.t()) :: boolean()
   def has_delegate_call?(config), do: Configuration.has_delegate_call?(config.fallback_config)
 
+  @spec max_signature_s(Configuration.t()) :: atom()
+  def max_signature_s(config), do: Configuration.max_signature_s(config.fallback_config)
+
   @spec fail_contract_creation_lack_of_gas?(Configuration.t()) :: boolean()
   def fail_contract_creation_lack_of_gas?(config),
     do: Configuration.fail_contract_creation_lack_of_gas?(config.fallback_config)

apps/evm/lib/evm/configuration/frontier.ex

@@ -2,6 +2,7 @@ defmodule EVM.Configuration.Frontier do
   defstruct contract_creation_cost: 21_000,
             has_delegate_call: false,
             fail_contract_creation: false,
+            max_signature_s: :secp256k1n,
             extcodesize_cost: 20,
             extcodecopy_cost: 20,
             balance_cost: 20,
@@ -27,6 +28,9 @@ defimpl EVM.Configuration, for: EVM.Configuration.Frontier do
   @spec has_delegate_call?(Configuration.t()) :: boolean()
   def has_delegate_call?(config), do: config.has_delegate_call
 
+  @spec max_signature_s(Configuration.t()) :: atom()
+  def max_signature_s(config), do: config.max_signature_s
+
   @spec fail_contract_creation_lack_of_gas?(Configuration.t()) :: boolean()
   def fail_contract_creation_lack_of_gas?(config), do: config.fail_contract_creation
 

apps/evm/lib/evm/configuration/homestead.ex

@@ -1,6 +1,7 @@
 defmodule EVM.Configuration.Homestead do
   defstruct contract_creation_cost: 53_000,
             has_delegate_call: true,
+            max_signature_s: :secp256k1n_2,
             fail_contract_creation: true,
             fallback_config: EVM.Configuration.Frontier.new()
 
@@ -18,6 +19,9 @@ defimpl EVM.Configuration, for: EVM.Configuration.Homestead do
   @spec has_delegate_call?(Configuration.t()) :: boolean()
   def has_delegate_call?(config), do: config.has_delegate_call
 
+  @spec max_signature_s(Configuration.t()) :: atom()
+  def max_signature_s(config), do: config.max_signature_s
+
   @spec fail_contract_creation_lack_of_gas?(Configuration.t()) :: boolean()
   def fail_contract_creation_lack_of_gas?(config), do: config.fail_contract_creation