From 6f7ec56bebf71727ec5a8162ad2fcaa15f28b4ac Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Fri, 19 May 2023 11:57:44 +0100 Subject: [PATCH 1/2] Discourage sending to Eth address on account layer --- src/masternodes/mn_checks.cpp | 30 ++++++++++++++++-------------- src/masternodes/mn_checks.h | 1 + src/masternodes/rpc_accounts.cpp | 22 ++++++++++++++++++++++ src/masternodes/rpc_loan.cpp | 4 ++++ src/masternodes/rpc_poolpair.cpp | 15 +++++++++++++++ src/masternodes/rpc_proposals.cpp | 2 ++ src/masternodes/rpc_vault.cpp | 11 +++++++++++ src/rpc/rawtransaction_util.cpp | 7 +++++++ src/rpc/rawtransaction_util.h | 1 + src/script/standard.cpp | 2 +- test/functional/feature_evm.py | 3 ++- 11 files changed, 82 insertions(+), 16 deletions(-) diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp index a0c71538475..cd26b181908 100644 --- a/src/masternodes/mn_checks.cpp +++ b/src/masternodes/mn_checks.cpp @@ -495,6 +495,21 @@ Res CCustomTxVisitor::HasAuth(const CScript &auth) const { return Res::Err("tx must have at least one input from account owner"); } +Res CCustomTxVisitor::HasEthAuth(const CScript &auth) const { + for (const auto &input : tx.vin) { + const Coin &coin = coins.AccessCoin(input.prevout); + std::vector vRet; + if (Solver(coin.out.scriptPubKey, vRet) == txnouttype::TX_PUBKEYHASH) { + auto it = input.scriptSig.begin(); + CPubKey pubkey(input.scriptSig.begin() + *it + 2, input.scriptSig.end()); + if (GetScriptForDestination(WitnessV16EthHash(pubkey)) == auth) { + return Res::Ok(); + } + } + } + return Res::Err("tx must have at least one input from account owner"); +} + Res CCustomTxVisitor::HasCollateralAuth(const uint256 &collateralTx) const { const Coin &auth = coins.AccessCoin(COutPoint(collateralTx, 1)); // always n=1 output Require(HasAuth(auth.out.scriptPubKey), "tx must have at least one input from the owner"); @@ -3870,20 +3885,7 @@ class CCustomTxApplyVisitor : public CCustomTxVisitor { return Res::Err("From address must be an ETH address in case of \"evmout\" transfer type"); } } - bool foundAuth = false; - for (const auto &input : tx.vin) { - const Coin &coin = coins.AccessCoin(input.prevout); - std::vector vRet; - if (Solver(coin.out.scriptPubKey, vRet) == txnouttype::TX_PUBKEYHASH) - { - auto it = input.scriptSig.begin(); - CPubKey pubkey(input.scriptSig.begin() + *it + 2, input.scriptSig.end()); - auto script = GetScriptForDestination(WitnessV16EthHash(pubkey)); - if (script == addr) - foundAuth = true; - } - } - if (!foundAuth) + if (!HasEthAuth(addr)) return Res::Err("authorization not found for %s in the tx", ScriptToString(addr)); const auto fromAddress = std::get(dest); diff --git a/src/masternodes/mn_checks.h b/src/masternodes/mn_checks.h index a572792f768..0326198b574 100644 --- a/src/masternodes/mn_checks.h +++ b/src/masternodes/mn_checks.h @@ -37,6 +37,7 @@ class CCustomTxVisitor { protected: Res HasAuth(const CScript &auth) const; + Res HasEthAuth(const CScript &auth) const; Res HasCollateralAuth(const uint256 &collateralTx) const; Res HasFoundationAuth() const; Res CheckMasternodeCreationTx() const; diff --git a/src/masternodes/rpc_accounts.cpp b/src/masternodes/rpc_accounts.cpp index 1e56f24dac7..35a74041dc9 100644 --- a/src/masternodes/rpc_accounts.cpp +++ b/src/masternodes/rpc_accounts.cpp @@ -655,6 +655,10 @@ UniValue utxostoaccount(const JSONRPCRequest& request) { CUtxosToAccountMessage msg{}; msg.to = DecodeRecipientsDefaultInternal(pwallet, request.params[0].get_obj()); + for (const auto& [to, amount] : msg.to) { + RejectEthAddress(to); + } + // encode CDataStream markedMetadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); markedMetadata << static_cast(CustomTxType::UtxosToAccount) @@ -809,6 +813,11 @@ UniValue accounttoaccount(const JSONRPCRequest& request) { msg.from = DecodeScript(request.params[0].get_str()); + for (const auto& [to, amount] : msg.to) { + RejectEthAddress(to); + } + RejectEthAddress(msg.from); + // encode CDataStream markedMetadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); markedMetadata << static_cast(CustomTxType::AccountToAccount) @@ -895,6 +904,7 @@ UniValue accounttoutxos(const JSONRPCRequest& request) { // decode sender and recipients CAccountToUtxosMessage msg{}; msg.from = DecodeScript(request.params[0].get_str()); + RejectEthAddress(msg.from); const auto to = DecodeRecipients(pwallet->chain(), request.params[1]); msg.balances = SumAllTransfers(to); if (msg.balances.balances.empty()) { @@ -1902,6 +1912,13 @@ UniValue sendtokenstoaddress(const JSONRPCRequest& request) { msg.from = DecodeRecipients(pwallet->chain(), request.params[0].get_obj()); } + for (const auto& [to, amount] : msg.to) { + RejectEthAddress(to); + } + for (const auto& [from, amount] : msg.from) { + RejectEthAddress(from); + } + // encode CDataStream markedMetadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); markedMetadata << static_cast(CustomTxType::AnyAccountsToAccounts) @@ -2357,6 +2374,7 @@ UniValue HandleSendDFIP2201BTCInput(const JSONRPCRequest& request, CWalletCoinsU throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); } const auto script = GetScriptForDestination(dest); + RejectEthAddress(script); CSmartContractMessage msg{}; msg.name = contractPair.first; @@ -2498,6 +2516,8 @@ UniValue futureswap(const JSONRPCRequest& request) { msg.owner = GetScriptForDestination(dest); msg.source = DecodeAmount(pwallet->chain(), request.params[1], ""); + RejectEthAddress(msg.owner); + if (!request.params[2].isNull()) { DCT_ID destTokenID{}; @@ -2600,6 +2620,8 @@ UniValue withdrawfutureswap(const JSONRPCRequest& request) { msg.destination = destTokenID.v; } + RejectEthAddress(msg.owner); + // Encode CDataStream metadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); metadata << static_cast(CustomTxType::FutureSwap) diff --git a/src/masternodes/rpc_loan.cpp b/src/masternodes/rpc_loan.cpp index 1f03e1e939c..a9f1efbc89d 100644 --- a/src/masternodes/rpc_loan.cpp +++ b/src/masternodes/rpc_loan.cpp @@ -1075,6 +1075,8 @@ UniValue takeloan(const JSONRPCRequest& request) { if (!metaObj["to"].isNull()) takeLoan.to = DecodeScript(metaObj["to"].getValStr()); + RejectEthAddress(takeLoan.to); + if (!metaObj["amounts"].isNull()) takeLoan.amounts = DecodeAmounts(pwallet->chain(), metaObj["amounts"], ""); else @@ -1265,6 +1267,8 @@ UniValue paybackloan(const JSONRPCRequest& request) { } else from = DecodeScript(metaObj["from"].getValStr()); + RejectEthAddress(from); + if (!::IsMine(*pwallet, from)) throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Address (%s) is not owned by the wallet", metaObj["from"].getValStr())); diff --git a/src/masternodes/rpc_poolpair.cpp b/src/masternodes/rpc_poolpair.cpp index 43540c1353b..13557679897 100644 --- a/src/masternodes/rpc_poolpair.cpp +++ b/src/masternodes/rpc_poolpair.cpp @@ -404,6 +404,11 @@ UniValue addpoolliquidity(const JSONRPCRequest &request) { } msg.shareAddress = DecodeScript(request.params[1].get_str()); + for (const auto& [from, balance] : msg.from) { + RejectEthAddress(from); + } + RejectEthAddress(msg.shareAddress); + // encode CDataStream markedMetadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); markedMetadata << static_cast(CustomTxType::AddPoolLiquidity) << msg; @@ -497,6 +502,8 @@ UniValue removepoolliquidity(const JSONRPCRequest &request) { msg.from = DecodeScript(from); msg.amount = DecodeAmount(pwallet->chain(), amount, from); + RejectEthAddress(msg.from); + // encode CDataStream markedMetadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); markedMetadata << static_cast(CustomTxType::RemovePoolLiquidity) << msg; @@ -636,6 +643,7 @@ UniValue createpoolpair(const JSONRPCRequest &request) { if (!metadataObj["customRewards"].isNull()) { rewards = DecodeAmounts(pwallet->chain(), metadataObj["customRewards"], ""); } + RejectEthAddress(ownerAddress); int targetHeight; DCT_ID idtokenA, idtokenB; @@ -815,6 +823,7 @@ UniValue updatepoolpair(const JSONRPCRequest &request) { std::numeric_limits::max())); } } + RejectEthAddress(ownerAddress); const auto txVersion = GetTransactionVersion(targetHeight); CMutableTransaction rawTx(txVersion); @@ -932,6 +941,9 @@ UniValue poolswap(const JSONRPCRequest &request) { CheckAndFillPoolSwapMessage(request, poolSwapMsg); int targetHeight = chainHeight(*pwallet->chain().lock()) + 1; + RejectEthAddress(poolSwapMsg.from); + RejectEthAddress(poolSwapMsg.to); + CDataStream metadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); metadata << static_cast(CustomTxType::PoolSwap); metadata << poolSwapMsg; @@ -1050,6 +1062,9 @@ UniValue compositeswap(const JSONRPCRequest &request) { CPoolSwapMessage &poolSwapMsg = poolSwapMsgV2.swapInfo; CheckAndFillPoolSwapMessage(request, poolSwapMsg); + RejectEthAddress(poolSwapMsg.from); + RejectEthAddress(poolSwapMsg.to); + { LOCK(cs_main); // If no direct swap found search for composite swap diff --git a/src/masternodes/rpc_proposals.cpp b/src/masternodes/rpc_proposals.cpp index 658715494a0..8b3009f516e 100644 --- a/src/masternodes/rpc_proposals.cpp +++ b/src/masternodes/rpc_proposals.cpp @@ -269,6 +269,8 @@ UniValue creategovcfp(const JSONRPCRequest &request) { pm.contextHash = contextHash; pm.options = 0; + RejectEthAddress(pm.address); + // encode CDataStream metadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); metadata << static_cast(CustomTxType::CreateCfp) << pm; diff --git a/src/masternodes/rpc_vault.cpp b/src/masternodes/rpc_vault.cpp index 409861b92c9..7627401dd23 100644 --- a/src/masternodes/rpc_vault.cpp +++ b/src/masternodes/rpc_vault.cpp @@ -303,6 +303,8 @@ UniValue createvault(const JSONRPCRequest& request) { CVaultMessage vault; vault.ownerAddress = DecodeScript(request.params[0].getValStr()); + RejectEthAddress(vault.ownerAddress); + if (request.params.size() > 1) { if (!request.params[1].isNull()) { vault.schemeId = request.params[1].get_str(); @@ -407,6 +409,8 @@ UniValue closevault(const JSONRPCRequest& request) { msg.to = DecodeScript(request.params[1].getValStr()); + RejectEthAddress(msg.to); + CDataStream metadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); metadata << static_cast(CustomTxType::CloseVault) << msg; @@ -713,6 +717,9 @@ UniValue updatevault(const JSONRPCRequest& request) { } msg.ownerAddress = DecodeScript(ownerAddress); } + + RejectEthAddress(msg.ownerAddress); + if(!params["loanSchemeId"].isNull()){ auto loanschemeid = params["loanSchemeId"].getValStr(); msg.schemeId = loanschemeid; @@ -801,6 +808,7 @@ UniValue deposittovault(const JSONRPCRequest& request) { // decode vaultId CVaultId vaultId = ParseHashV(request.params[0], "vaultId"); auto from = DecodeScript(request.params[1].get_str()); + RejectEthAddress(from); CTokenAmount amount = DecodeAmount(pwallet->chain(),request.params[2].get_str(), "amount"); CDepositToVaultMessage msg{vaultId, from, amount}; @@ -887,6 +895,7 @@ UniValue withdrawfromvault(const JSONRPCRequest& request) { // decode vaultId CVaultId vaultId = ParseHashV(request.params[0], "vaultId"); auto to = DecodeScript(request.params[1].get_str()); + RejectEthAddress(to); CTokenAmount amount = DecodeAmount(pwallet->chain(),request.params[2].get_str(), "amount"); CWithdrawFromVaultMessage msg{vaultId, to, amount}; @@ -1005,6 +1014,8 @@ UniValue placeauctionbid(const JSONRPCRequest& request) { from = DecodeScript(fromStr); } + RejectEthAddress(from); + CAuctionBidMessage msg{vaultId, index, from, amount}; CDataStream markedMetadata(DfTxMarker, SER_NETWORK, PROTOCOL_VERSION); markedMetadata << static_cast(CustomTxType::AuctionBid) diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index 427da3ccc47..33bc2195608 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -86,6 +86,13 @@ CScript DecodeScript(std::string const& str) return GetScriptForDestination(dest); } +void RejectEthAddress(const CScript &address) { + CTxDestination dest; + if (ExtractDestination(address, dest) && dest.index() == WitV16KeyEthHashType) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Eth type addresses are not valid"); + } +} + int DecodeScriptTxId(const std::string& str, CParserResults result) { if (IsHex(str)) { diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h index a5b26107796..b10afad3ad8 100644 --- a/src/rpc/rawtransaction_util.h +++ b/src/rpc/rawtransaction_util.h @@ -28,6 +28,7 @@ ResVal> ParseTokenAmount(std::string const & tok ResVal GuessTokenAmount(interfaces::Chain const & chain,std::string const & tokenAmount); CScript DecodeScript(std::string const& str); +void RejectEthAddress(const CScript &address); CTokenAmount DecodeAmount(interfaces::Chain const & chain, UniValue const& amountUni, std::string const& name); CBalances DecodeAmounts(interfaces::Chain const & chain, UniValue const& amountsUni, std::string const& name); CAccounts DecodeRecipients(interfaces::Chain const & chain, UniValue const& sendTo); diff --git a/src/script/standard.cpp b/src/script/standard.cpp index d18cace75cc..6c0eb610c7e 100644 --- a/src/script/standard.cpp +++ b/src/script/standard.cpp @@ -351,5 +351,5 @@ CScript GetScriptForHTLC(const CPubKey& seller, const CPubKey& refund, const std } bool IsValidDestination(const CTxDestination& dest) { - return dest.index() != 0; + return dest.index() != NoDestType; } diff --git a/test/functional/feature_evm.py b/test/functional/feature_evm.py index bcff856e89e..da97d532d29 100755 --- a/test/functional/feature_evm.py +++ b/test/functional/feature_evm.py @@ -91,7 +91,8 @@ def run_test(self): assert_equal(ETHbalance, int_to_eth_u256(0)) assert_equal(len(self.nodes[0].getaccount(ethAddress, {}, True)), 0) - assert_raises_rpc_error(-3, "xpected type number, got string", self.nodes[0].transferdomain, "blabla", {address:["100@DFI"]}, {ethAddress:["100@DFI"]}) + assert_raises_rpc_error(-5, "Eth type addresses are not valid", self.nodes[0].accounttoaccount, address, {ethAddress: "1@DFI"}) + assert_raises_rpc_error(-3, "Expected type number, got string", self.nodes[0].transferdomain, "blabla", {address:["100@DFI"]}, {ethAddress:["100@DFI"]}) assert_raises_rpc_error(-8, "Invalid parameters, argument \"type\" must be either 1 (DFI token to EVM) or 2 (EVM to DFI token)", self.nodes[0].transferdomain, 0, {address:["100@DFI"]}, {ethAddress:["100@DFI"]}) assert_raises_rpc_error(-5, "recipient (blablabla) does not refer to any valid address", self.nodes[0].transferdomain, 1, {"blablabla":["100@DFI"]}, {ethAddress:["100@DFI"]}) assert_raises_rpc_error(-5, "recipient (blablabla) does not refer to any valid address", self.nodes[0].transferdomain, 1, {address:["100@DFI"]}, {"blablabla":["100@DFI"]}) From 47a65b814b8c847a230c048e4a469c2bc794b6ad Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Fri, 9 Jun 2023 11:56:30 +0100 Subject: [PATCH 2/2] Remove unused function and add test --- src/masternodes/mn_checks.cpp | 15 --------------- src/masternodes/mn_checks.h | 1 - test/functional/feature_evm.py | 1 + 3 files changed, 1 insertion(+), 16 deletions(-) diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp index b80f9f57779..83959251f28 100644 --- a/src/masternodes/mn_checks.cpp +++ b/src/masternodes/mn_checks.cpp @@ -491,21 +491,6 @@ Res CCustomTxVisitor::HasAuth(const CScript &auth) const { return ::HasAuth(tx, coins, auth); } -Res CCustomTxVisitor::HasEthAuth(const CScript &auth) const { - for (const auto &input : tx.vin) { - const Coin &coin = coins.AccessCoin(input.prevout); - std::vector vRet; - if (Solver(coin.out.scriptPubKey, vRet) == txnouttype::TX_PUBKEYHASH) { - auto it = input.scriptSig.begin(); - CPubKey pubkey(input.scriptSig.begin() + *it + 2, input.scriptSig.end()); - if (GetScriptForDestination(WitnessV16EthHash(pubkey)) == auth) { - return Res::Ok(); - } - } - } - return Res::Err("tx must have at least one input from account owner"); -} - Res CCustomTxVisitor::HasCollateralAuth(const uint256 &collateralTx) const { const Coin &auth = coins.AccessCoin(COutPoint(collateralTx, 1)); // always n=1 output Require(HasAuth(auth.out.scriptPubKey), "tx must have at least one input from the owner"); diff --git a/src/masternodes/mn_checks.h b/src/masternodes/mn_checks.h index 072906a566c..ef2301f3ef6 100644 --- a/src/masternodes/mn_checks.h +++ b/src/masternodes/mn_checks.h @@ -37,7 +37,6 @@ class CCustomTxVisitor { protected: Res HasAuth(const CScript &auth) const; - Res HasEthAuth(const CScript &auth) const; Res HasCollateralAuth(const uint256 &collateralTx) const; Res HasFoundationAuth() const; Res CheckMasternodeCreationTx() const; diff --git a/test/functional/feature_evm.py b/test/functional/feature_evm.py index 11228f5452a..a9da18a3b83 100755 --- a/test/functional/feature_evm.py +++ b/test/functional/feature_evm.py @@ -110,6 +110,7 @@ def run_test(self): assert_equal(len(self.nodes[0].getaccount(ethAddress, {}, True)), 0) # Check for invalid parameters in transferdomain rpc + assert_raises_rpc_error(-5, "Eth type addresses are not valid", self.nodes[0].accounttoaccount, address, {ethAddress: "1@DFI"}) assert_raises_rpc_error(-8, "Invalid parameters, src argument \"address\" must not be null", self.nodes[0].transferdomain, [{"src": {"amount":"100@DFI", "domain": 2}, "dst":{"address":ethAddress, "amount":"100@DFI", "domain": 3}}]) assert_raises_rpc_error(-8, "Invalid parameters, src argument \"amount\" must not be null", self.nodes[0].transferdomain, [{"src": {"address":address, "domain": 2}, "dst":{"address":ethAddress, "amount":"100@DFI", "domain": 3}}]) assert_raises_rpc_error(-8, "Invalid parameters, src argument \"domain\" must not be null", self.nodes[0].transferdomain, [{"src": {"address":address, "amount":"100@DFI"}, "dst":{"address":ethAddress, "amount":"100@DFI", "domain": 3}}])