From 73c0dfadd9a8ff2864205b62cbb1db228cd70960 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Mon, 26 Jun 2023 08:28:49 +0100 Subject: [PATCH 1/4] Handle ExtractDestination failure --- src/masternodes/mn_checks.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp index e20c125202..2b84894d1e 100644 --- a/src/masternodes/mn_checks.cpp +++ b/src/masternodes/mn_checks.cpp @@ -4013,6 +4013,8 @@ Res ValidateTransferDomain(const CTransaction &tx, if (dest.index() == WitV16KeyEthHashType) { return DeFiErrors::TransferDomainETHSourceAddress(); } + } else { + return DeFiErrors::TransferDomainETHSourceAddress(); } // Check for authorization on source address res = HasAuth(tx, coins, src.address); @@ -4024,6 +4026,8 @@ Res ValidateTransferDomain(const CTransaction &tx, if (dest.index() != WitV16KeyEthHashType) { return DeFiErrors::TransferDomainDFISourceAddress(); } + } else { + return DeFiErrors::TransferDomainDFISourceAddress(); } // Check for authorization on source address res = HasAuth(tx, coins, src.address, AuthStrategy::EthKeyMatch); @@ -4040,6 +4044,8 @@ Res ValidateTransferDomain(const CTransaction &tx, if (dest.index() == WitV16KeyEthHashType) { return DeFiErrors::TransferDomainETHDestinationAddress(); } + } else { + return DeFiErrors::TransferDomainETHDestinationAddress(); } } else if (dst.domain == static_cast(VMDomain::EVM)) { // Reject if source address is DFI address @@ -4047,6 +4053,8 @@ Res ValidateTransferDomain(const CTransaction &tx, if (dest.index() != WitV16KeyEthHashType) { return DeFiErrors::TransferDomainDVMDestinationAddress(); } + } else { + return DeFiErrors::TransferDomainDVMDestinationAddress(); } } else return DeFiErrors::TransferDomainInvalidDestinationDomain(); From d43eb7a0e255c7686754b3991a5d39c1725815c7 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Mon, 26 Jun 2023 11:16:05 +0100 Subject: [PATCH 2/4] Fork guard ExtractDestination failure --- src/masternodes/mn_checks.cpp | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp index 2b84894d1e..ecdfb595ad 100644 --- a/src/masternodes/mn_checks.cpp +++ b/src/masternodes/mn_checks.cpp @@ -4014,7 +4014,10 @@ Res ValidateTransferDomain(const CTransaction &tx, return DeFiErrors::TransferDomainETHSourceAddress(); } } else { - return DeFiErrors::TransferDomainETHSourceAddress(); + // Remove guard on mainnet release + if (height > static_cast(consensus.ChangiIntermediateHeight3)) { + return DeFiErrors::TransferDomainETHSourceAddress(); + } } // Check for authorization on source address res = HasAuth(tx, coins, src.address); @@ -4027,7 +4030,10 @@ Res ValidateTransferDomain(const CTransaction &tx, return DeFiErrors::TransferDomainDFISourceAddress(); } } else { - return DeFiErrors::TransferDomainDFISourceAddress(); + // Remove guard on mainnet release + if (height > static_cast(consensus.ChangiIntermediateHeight3)) { + return DeFiErrors::TransferDomainDFISourceAddress(); + } } // Check for authorization on source address res = HasAuth(tx, coins, src.address, AuthStrategy::EthKeyMatch); @@ -4045,7 +4051,10 @@ Res ValidateTransferDomain(const CTransaction &tx, return DeFiErrors::TransferDomainETHDestinationAddress(); } } else { - return DeFiErrors::TransferDomainETHDestinationAddress(); + // Remove guard on mainnet release + if (height > static_cast(consensus.ChangiIntermediateHeight3)) { + return DeFiErrors::TransferDomainETHDestinationAddress(); + } } } else if (dst.domain == static_cast(VMDomain::EVM)) { // Reject if source address is DFI address @@ -4054,7 +4063,10 @@ Res ValidateTransferDomain(const CTransaction &tx, return DeFiErrors::TransferDomainDVMDestinationAddress(); } } else { - return DeFiErrors::TransferDomainDVMDestinationAddress(); + // Remove guard on mainnet release + if (height > static_cast(consensus.ChangiIntermediateHeight3)) { + return DeFiErrors::TransferDomainDVMDestinationAddress(); + } } } else return DeFiErrors::TransferDomainInvalidDestinationDomain(); From fbac242c2c48c3028bd2fca1bcdb2fcea21eb86c Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Mon, 26 Jun 2023 12:34:56 +0100 Subject: [PATCH 3/4] Limit DVM transfers to PKHash and Bech32 --- src/masternodes/errors.h | 6 ++-- src/masternodes/mn_checks.cpp | 66 +++++++++++++++++++++++----------- test/functional/feature_evm.py | 10 +++--- 3 files changed, 54 insertions(+), 28 deletions(-) diff --git a/src/masternodes/errors.h b/src/masternodes/errors.h index fb706e5a46..0f43209fe4 100644 --- a/src/masternodes/errors.h +++ b/src/masternodes/errors.h @@ -427,7 +427,7 @@ class DeFiErrors { } static Res TransferDomainETHSourceAddress() { - return Res::Err("Src address must not be an ETH address in case of \"DVM\" domain"); + return Res::Err("Src address must be a legacy or Bech32 address in case of \"DVM\" domain"); } static Res TransferDomainDFISourceAddress() { @@ -439,11 +439,11 @@ class DeFiErrors { } static Res TransferDomainETHDestinationAddress() { - return Res::Err("Dst address must not be an ETH address in case of \"DVM\" domain"); + return Res::Err("Dst address must be a legacy or Bech32 address in case of \"DVM\" domain"); } static Res TransferDomainDVMDestinationAddress() { - return Res::Err("Dst address must be an ETH address in case of \"EVM\" domain"); + return Res::Err("Dst address must be a legacy or Bech32 address in case of \"EVM\" domain"); } static Res TransferDomainInvalidDestinationDomain() { diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp index ecdfb595ad..b01f513a5f 100644 --- a/src/masternodes/mn_checks.cpp +++ b/src/masternodes/mn_checks.cpp @@ -3975,6 +3975,28 @@ Res HasAuth(const CTransaction &tx, const CCoinsViewCache &coins, const CScript return DeFiErrors::InvalidAuth(); } +enum class DomainTransferType : uint8_t { + DVM, + EVM, +}; + +static bool DomainTransferAllowedAddress(const CScript &address, DomainTransferType type) { + CTxDestination dest; + if (ExtractDestination(address, dest)) { + if (type == DomainTransferType::DVM) { + if (dest.index() == PKHashType || dest.index() == WitV0KeyHashType) { + return true; + } + } else if (type == DomainTransferType::EVM) { + if (dest.index() == WitV16KeyEthHashType) { + return true; + } + } + } + + return false; +} + Res ValidateTransferDomain(const CTransaction &tx, uint32_t height, const CCoinsViewCache &coins, @@ -4009,13 +4031,14 @@ Res ValidateTransferDomain(const CTransaction &tx, // Check domain type if (src.domain == static_cast(VMDomain::DVM)) { // Reject if source address is ETH address - if (ExtractDestination(src.address, dest)) { - if (dest.index() == WitV16KeyEthHashType) { - return DeFiErrors::TransferDomainETHSourceAddress(); + if (height < static_cast(consensus.ChangiIntermediateHeight3)) { + if (ExtractDestination(src.address, dest)) { + if (dest.index() == WitV16KeyEthHashType) { + return DeFiErrors::TransferDomainETHSourceAddress(); + } } } else { - // Remove guard on mainnet release - if (height > static_cast(consensus.ChangiIntermediateHeight3)) { + if (!DomainTransferAllowedAddress(src.address, DomainTransferType::DVM)) { return DeFiErrors::TransferDomainETHSourceAddress(); } } @@ -4025,13 +4048,14 @@ Res ValidateTransferDomain(const CTransaction &tx, return res; } else if (src.domain == static_cast(VMDomain::EVM)) { // Reject if source address is DFI address - if (ExtractDestination(src.address, dest)) { - if (dest.index() != WitV16KeyEthHashType) { - return DeFiErrors::TransferDomainDFISourceAddress(); + if (height < static_cast(consensus.ChangiIntermediateHeight3)) { + if (ExtractDestination(src.address, dest)) { + if (dest.index() != WitV16KeyEthHashType) { + return DeFiErrors::TransferDomainDFISourceAddress(); + } } } else { - // Remove guard on mainnet release - if (height > static_cast(consensus.ChangiIntermediateHeight3)) { + if (!DomainTransferAllowedAddress(src.address, DomainTransferType::EVM)) { return DeFiErrors::TransferDomainDFISourceAddress(); } } @@ -4046,25 +4070,27 @@ Res ValidateTransferDomain(const CTransaction &tx, // Check domain type if (dst.domain == static_cast(VMDomain::DVM)) { // Reject if source address is ETH address - if (ExtractDestination(dst.address, dest)) { - if (dest.index() == WitV16KeyEthHashType) { - return DeFiErrors::TransferDomainETHDestinationAddress(); + if (height < static_cast(consensus.ChangiIntermediateHeight3)) { + if (ExtractDestination(dst.address, dest)) { + if (dest.index() == WitV16KeyEthHashType) { + return DeFiErrors::TransferDomainETHDestinationAddress(); + } } } else { - // Remove guard on mainnet release - if (height > static_cast(consensus.ChangiIntermediateHeight3)) { + if (!DomainTransferAllowedAddress(dst.address, DomainTransferType::DVM)) { return DeFiErrors::TransferDomainETHDestinationAddress(); } } } else if (dst.domain == static_cast(VMDomain::EVM)) { // Reject if source address is DFI address - if (ExtractDestination(dst.address, dest)) { - if (dest.index() != WitV16KeyEthHashType) { - return DeFiErrors::TransferDomainDVMDestinationAddress(); + if (height < static_cast(consensus.ChangiIntermediateHeight3)) { + if (ExtractDestination(dst.address, dest)) { + if (dest.index() != WitV16KeyEthHashType) { + return DeFiErrors::TransferDomainDVMDestinationAddress(); + } } } else { - // Remove guard on mainnet release - if (height > static_cast(consensus.ChangiIntermediateHeight3)) { + if (!DomainTransferAllowedAddress(dst.address, DomainTransferType::EVM)) { return DeFiErrors::TransferDomainDVMDestinationAddress(); } } diff --git a/test/functional/feature_evm.py b/test/functional/feature_evm.py index f963e07b49..8ff744c318 100755 --- a/test/functional/feature_evm.py +++ b/test/functional/feature_evm.py @@ -20,8 +20,8 @@ def set_test_params(self): self.num_nodes = 2 self.setup_clean_chain = True self.extra_args = [ - ['-dummypos=0', '-txnotokens=0', '-amkheight=50', '-bayfrontheight=51', '-eunosheight=80', '-fortcanningheight=82', '-fortcanninghillheight=84', '-fortcanningroadheight=86', '-fortcanningcrunchheight=88', '-fortcanningspringheight=90', '-fortcanninggreatworldheight=94', '-fortcanningepilogueheight=96', '-grandcentralheight=101', '-nextnetworkupgradeheight=105', '-changiintermediateheight=105', '-subsidytest=1', '-txindex=1'], - ['-dummypos=0', '-txnotokens=0', '-amkheight=50', '-bayfrontheight=51', '-eunosheight=80', '-fortcanningheight=82', '-fortcanninghillheight=84', '-fortcanningroadheight=86', '-fortcanningcrunchheight=88', '-fortcanningspringheight=90', '-fortcanninggreatworldheight=94', '-fortcanningepilogueheight=96', '-grandcentralheight=101', '-nextnetworkupgradeheight=105', '-changiintermediateheight=105', '-subsidytest=1', '-txindex=1'] + ['-dummypos=0', '-txnotokens=0', '-amkheight=50', '-bayfrontheight=51', '-eunosheight=80', '-fortcanningheight=82', '-fortcanninghillheight=84', '-fortcanningroadheight=86', '-fortcanningcrunchheight=88', '-fortcanningspringheight=90', '-fortcanninggreatworldheight=94', '-fortcanningepilogueheight=96', '-grandcentralheight=101', '-nextnetworkupgradeheight=105', '-changiintermediateheight=105', '-changiintermediate3height=105', '-subsidytest=1', '-txindex=1'], + ['-dummypos=0', '-txnotokens=0', '-amkheight=50', '-bayfrontheight=51', '-eunosheight=80', '-fortcanningheight=82', '-fortcanninghillheight=84', '-fortcanningroadheight=86', '-fortcanningcrunchheight=88', '-fortcanningspringheight=90', '-fortcanninggreatworldheight=94', '-fortcanningepilogueheight=96', '-grandcentralheight=101', '-nextnetworkupgradeheight=105', '-changiintermediateheight=105', '-changiintermediate3height=105', '-subsidytest=1', '-txindex=1'] ] def test_tx_without_chainid(self, node, keypair): @@ -120,8 +120,8 @@ def run_test(self): assert_raises_rpc_error(-5, "recipient (blablabla) does not refer to any valid address", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 2}, "dst":{"address":"blablabla", "amount":"100@DFI", "domain": 3}}]) # Check for valid values DVM->EVM in transferdomain rpc - assert_raises_rpc_error(-32600, "Src address must not be an ETH address in case of \"DVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":eth_address, "amount":"100@DFI", "domain": 2}, "dst":{"address":eth_address, "amount":"100@DFI", "domain": 3}}]) - assert_raises_rpc_error(-32600, "Dst address must be an ETH address in case of \"EVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 2}, "dst":{"address":address, "amount":"100@DFI", "domain": 3}}]) + assert_raises_rpc_error(-32600, "Src address must be a legacy or Bech32 address in case of \"DVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":eth_address, "amount":"100@DFI", "domain": 2}, "dst":{"address":eth_address, "amount":"100@DFI", "domain": 3}}]) + assert_raises_rpc_error(-32600, "Dst address must be a legacy or Bech32 address in case of \"EVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 2}, "dst":{"address":address, "amount":"100@DFI", "domain": 3}}]) assert_raises_rpc_error(-32600, "Cannot transfer inside same domain", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 2}, "dst":{"address":eth_address, "amount":"100@DFI", "domain": 2}}]) assert_raises_rpc_error(-32600, "Source amount must be equal to destination amount", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 2}, "dst":{"address":eth_address, "amount":"101@DFI", "domain": 3}}]) assert_raises_rpc_error(-32600, "For transferdomain, only DFI token is currently supported", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@BTC", "domain": 2}, "dst":{"address":eth_address, "amount":"100@DFI", "domain": 3}}]) @@ -156,7 +156,7 @@ def run_test(self): # Check for valid values EVM->DVM in transferdomain rpc assert_raises_rpc_error(-32600, "Src address must be an ETH address in case of \"EVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 3}, "dst":{"address":address, "amount":"100@DFI", "domain": 2}}]) - assert_raises_rpc_error(-32600, "Dst address must not be an ETH address in case of \"DVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":eth_address, "amount":"100@DFI", "domain": 3}, "dst":{"address":eth_address, "amount":"100@DFI", "domain": 2}}]) + assert_raises_rpc_error(-32600, "Dst address must be a legacy or Bech32 address in case of \"DVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":eth_address, "amount":"100@DFI", "domain": 3}, "dst":{"address":eth_address, "amount":"100@DFI", "domain": 2}}]) assert_raises_rpc_error(-32600, "Cannot transfer inside same domain", self.nodes[0].transferdomain, [{"src": {"address":eth_address, "amount":"100@DFI", "domain": 3}, "dst":{"address":address, "amount":"100@DFI", "domain": 3}}]) assert_raises_rpc_error(-32600, "Source amount must be equal to destination amount", self.nodes[0].transferdomain, [{"src": {"address":eth_address, "amount":"100@DFI", "domain": 3}, "dst":{"address":address, "amount":"101@DFI", "domain": 2}}]) assert_raises_rpc_error(-32600, "For transferdomain, only DFI token is currently supported", self.nodes[0].transferdomain, [{"src": {"address":eth_address, "amount":"100@BTC", "domain": 3}, "dst":{"address":address, "amount":"100@DFI", "domain": 2}}]) From 6f168d636bb0d3b26f777ba7e069bb39bff90af7 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Mon, 26 Jun 2023 12:47:02 +0100 Subject: [PATCH 4/4] Correct error messages and test --- src/masternodes/errors.h | 8 ++++---- src/masternodes/mn_checks.cpp | 16 ++++++++-------- test/functional/feature_evm.py | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/masternodes/errors.h b/src/masternodes/errors.h index 0f43209fe4..58aeb04705 100644 --- a/src/masternodes/errors.h +++ b/src/masternodes/errors.h @@ -427,11 +427,11 @@ class DeFiErrors { } static Res TransferDomainETHSourceAddress() { - return Res::Err("Src address must be a legacy or Bech32 address in case of \"DVM\" domain"); + return Res::Err("Src address must be an ETH address in case of \"EVM\" domain"); } static Res TransferDomainDFISourceAddress() { - return Res::Err("Src address must be an ETH address in case of \"EVM\" domain"); + return Res::Err("Src address must be a legacy or Bech32 address in case of \"DVM\" domain"); } static Res TransferDomainInvalidSourceDomain() { @@ -439,11 +439,11 @@ class DeFiErrors { } static Res TransferDomainETHDestinationAddress() { - return Res::Err("Dst address must be a legacy or Bech32 address in case of \"DVM\" domain"); + return Res::Err("Dst address must be an ETH address in case of \"EVM\" domain"); } static Res TransferDomainDVMDestinationAddress() { - return Res::Err("Dst address must be a legacy or Bech32 address in case of \"EVM\" domain"); + return Res::Err("Dst address must be a legacy or Bech32 address in case of \"DVM\" domain"); } static Res TransferDomainInvalidDestinationDomain() { diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp index b01f513a5f..0002f36d42 100644 --- a/src/masternodes/mn_checks.cpp +++ b/src/masternodes/mn_checks.cpp @@ -4034,12 +4034,12 @@ Res ValidateTransferDomain(const CTransaction &tx, if (height < static_cast(consensus.ChangiIntermediateHeight3)) { if (ExtractDestination(src.address, dest)) { if (dest.index() == WitV16KeyEthHashType) { - return DeFiErrors::TransferDomainETHSourceAddress(); + return DeFiErrors::TransferDomainDFISourceAddress(); } } } else { if (!DomainTransferAllowedAddress(src.address, DomainTransferType::DVM)) { - return DeFiErrors::TransferDomainETHSourceAddress(); + return DeFiErrors::TransferDomainDFISourceAddress(); } } // Check for authorization on source address @@ -4051,12 +4051,12 @@ Res ValidateTransferDomain(const CTransaction &tx, if (height < static_cast(consensus.ChangiIntermediateHeight3)) { if (ExtractDestination(src.address, dest)) { if (dest.index() != WitV16KeyEthHashType) { - return DeFiErrors::TransferDomainDFISourceAddress(); + return DeFiErrors::TransferDomainETHSourceAddress(); } } } else { if (!DomainTransferAllowedAddress(src.address, DomainTransferType::EVM)) { - return DeFiErrors::TransferDomainDFISourceAddress(); + return DeFiErrors::TransferDomainETHSourceAddress(); } } // Check for authorization on source address @@ -4073,12 +4073,12 @@ Res ValidateTransferDomain(const CTransaction &tx, if (height < static_cast(consensus.ChangiIntermediateHeight3)) { if (ExtractDestination(dst.address, dest)) { if (dest.index() == WitV16KeyEthHashType) { - return DeFiErrors::TransferDomainETHDestinationAddress(); + return DeFiErrors::TransferDomainDVMDestinationAddress(); } } } else { if (!DomainTransferAllowedAddress(dst.address, DomainTransferType::DVM)) { - return DeFiErrors::TransferDomainETHDestinationAddress(); + return DeFiErrors::TransferDomainDVMDestinationAddress(); } } } else if (dst.domain == static_cast(VMDomain::EVM)) { @@ -4086,12 +4086,12 @@ Res ValidateTransferDomain(const CTransaction &tx, if (height < static_cast(consensus.ChangiIntermediateHeight3)) { if (ExtractDestination(dst.address, dest)) { if (dest.index() != WitV16KeyEthHashType) { - return DeFiErrors::TransferDomainDVMDestinationAddress(); + return DeFiErrors::TransferDomainETHDestinationAddress(); } } } else { if (!DomainTransferAllowedAddress(dst.address, DomainTransferType::EVM)) { - return DeFiErrors::TransferDomainDVMDestinationAddress(); + return DeFiErrors::TransferDomainETHDestinationAddress(); } } } else diff --git a/test/functional/feature_evm.py b/test/functional/feature_evm.py index 8ff744c318..891dae9f72 100755 --- a/test/functional/feature_evm.py +++ b/test/functional/feature_evm.py @@ -121,7 +121,7 @@ def run_test(self): # Check for valid values DVM->EVM in transferdomain rpc assert_raises_rpc_error(-32600, "Src address must be a legacy or Bech32 address in case of \"DVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":eth_address, "amount":"100@DFI", "domain": 2}, "dst":{"address":eth_address, "amount":"100@DFI", "domain": 3}}]) - assert_raises_rpc_error(-32600, "Dst address must be a legacy or Bech32 address in case of \"EVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 2}, "dst":{"address":address, "amount":"100@DFI", "domain": 3}}]) + assert_raises_rpc_error(-32600, "Dst address must be an ETH address in case of \"EVM\" domain", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 2}, "dst":{"address":address, "amount":"100@DFI", "domain": 3}}]) assert_raises_rpc_error(-32600, "Cannot transfer inside same domain", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 2}, "dst":{"address":eth_address, "amount":"100@DFI", "domain": 2}}]) assert_raises_rpc_error(-32600, "Source amount must be equal to destination amount", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI", "domain": 2}, "dst":{"address":eth_address, "amount":"101@DFI", "domain": 3}}]) assert_raises_rpc_error(-32600, "For transferdomain, only DFI token is currently supported", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@BTC", "domain": 2}, "dst":{"address":eth_address, "amount":"100@DFI", "domain": 3}}])