diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index 878aa296dd..8636c99284 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -535,8 +535,10 @@ STTx::checkMultiSign(Rules const& rules, STObject const& sigObject) const { // Used inside the loop in multiSignHelper to enforce that // the account owner may not multisign for themselves. + // For delegated transactions sfDelegate is the account whose signer list is checked, + // the delegate account itself can not be among the signers. auto const txnAccountID = - &sigObject != this ? std::nullopt : std::optional(getAccountID(sfAccount)); + &sigObject != this ? std::nullopt : std::optional(getFeePayer()); // We can ease the computational load inside the loop a bit by // pre-constructing part of the data that we hash. Fill a Serializer diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index 06884e9931..588aeee634 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -1879,6 +1880,149 @@ class Delegate_test : public beast::unit_test::Suite BEAST_EXPECT(env.balance(edward) == edwardBalance); } + void + testMultiSignDelegatorAsSigner() + { + // checkMultiSign disallows the owner of the account to + // be part of the multisigner list. When it is a delegated transaction, + // the delegate account should not be part of the multisigner list. + testcase("test delegator as multisigner in delegate's signer list"); + using namespace jtx; + + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const carol{"carol"}; + Account const daria{"daria"}; + env.fund(XRP(100000), alice, bob, carol, daria); + env.close(); + + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + // bob's signer list includes the delegator alice and daria + env(signers(bob, 2, {{alice, 1}, {daria, 1}})); + env.close(); + + auto aliceBalance = env.balance(alice); + auto bobBalance = env.balance(bob); + auto carolBalance = env.balance(carol); + auto const amt = 100; + + // alice can sign as a multisigner for bob + env(pay(alice, carol, XRP(100)), Fee(XRP(10)), delegate::As(bob), Msig(alice, daria)); + env.close(); + + BEAST_EXPECT(env.balance(alice) == aliceBalance - XRP(amt)); + BEAST_EXPECT(env.balance(bob) == bobBalance - XRP(10)); + BEAST_EXPECT(env.balance(carol) == carolBalance + XRP(amt)); + + // alice can not sign as a multisigner if she sent the transaction by herself. + env(pay(alice, carol, XRP(100)), Fee(XRP(10)), Msig(alice, daria), Ter(telENV_RPC_FAILED)); + env.close(); + + // Get new balances + aliceBalance = env.balance(alice); + bobBalance = env.balance(bob); + carolBalance = env.balance(carol); + + // bob (the delegate) should not appear as a multisigner in his transaction sent on behalf + // of alice. STTx::checkMultiSign catches this at the local-check stage, so the jtx + // framework returns telENV_RPC_FAILED. + env(pay(alice, carol, XRP(50)), + Fee(XRP(10)), + delegate::As(bob), + Msig(alice, bob), + Ter(telENV_RPC_FAILED)); + env.close(); + BEAST_EXPECT(env.balance(alice) == aliceBalance); + BEAST_EXPECT(env.balance(bob) == bobBalance); + BEAST_EXPECT(env.balance(carol) == carolBalance); + } + + void + testSignForDelegated() + { + // In sortAndValidateSigners, if it is a delegated transaction, the delegate account is + // the forbidden account from appearing in its own Signers array. + testcase("test sign_for with delegated transaction"); + using namespace jtx; + + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const carol{"carol"}; + Account const daria{"daria"}; + env.fund(XRP(100000), alice, bob, carol, daria); + env.close(); + + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + // bob's signer list includes the delegator alice and daria + env(signers(bob, 2, {{alice, 1}, {daria, 1}})); + env.close(); + + auto const baseFee = env.current()->fees().base; + + auto const sendAmt = 1'000'000; + auto makeDelegateTx = [&]() -> json::Value { + json::Value jv; + jv[jss::tx_json][jss::Account] = alice.human(); + jv[jss::tx_json][sfDelegate.jsonName] = bob.human(); + jv[jss::tx_json][jss::TransactionType] = jss::Payment; + jv[jss::tx_json][jss::Destination] = carol.human(); + jv[jss::tx_json][jss::Amount] = sendAmt; + jv[jss::tx_json][jss::Fee] = std::to_string((10 * baseFee).drops()); + jv[jss::tx_json][jss::Sequence] = env.seq(alice); + jv[jss::tx_json][jss::SigningPubKey] = ""; + return jv; + }; + + // The delegator alice and daria both sign via sign_for, which is valid + { + auto const aliceBalance = env.balance(alice); + auto const bobBalance = env.balance(bob); + auto const dariaBalance = env.balance(daria); + auto const carolBalance = env.balance(carol); + + json::Value jv = makeDelegateTx(); + jv[jss::account] = alice.human(); + jv[jss::secret] = alice.name(); + auto jrr = env.rpc("json", "sign_for", to_string(jv))[jss::result]; + BEAST_EXPECT(jrr[jss::status] == "success"); + + json::Value jv2; + jv2[jss::tx_json] = jrr[jss::tx_json]; + jv2[jss::account] = daria.human(); + jv2[jss::secret] = daria.name(); + jrr = env.rpc("json", "sign_for", to_string(jv2))[jss::result]; + BEAST_EXPECT(jrr[jss::status] == "success"); + + json::Value jvSubmit; + jvSubmit[jss::tx_json] = jrr[jss::tx_json]; + jrr = env.rpc("json", "submit_multisigned", to_string(jvSubmit))[jss::result]; + BEAST_EXPECT(jrr[jss::status] == "success"); + env.close(); + BEAST_EXPECT(env.balance(alice) == aliceBalance - XRPAmount(sendAmt)); + BEAST_EXPECT(env.balance(bob) == bobBalance - (10 * baseFee)); + BEAST_EXPECT(env.balance(daria) == dariaBalance); + BEAST_EXPECT(env.balance(carol) == carolBalance + XRPAmount(sendAmt)); + } + + // The delegated account bob attempts sign_for, will be rejected. + { + json::Value jv = makeDelegateTx(); + jv[jss::account] = bob.human(); + jv[jss::secret] = bob.name(); + auto jrr = env.rpc("json", "sign_for", to_string(jv))[jss::result]; + BEAST_EXPECT(jrr[jss::status] == "error"); + BEAST_EXPECT( + jrr[jss::error_message].asString().find( + "A Signer may not be the transaction's Account") != std::string::npos); + } + } + void testPermissionValue(FeatureBitset features) { @@ -2086,6 +2230,8 @@ class Delegate_test : public beast::unit_test::Suite testSingleSignBadSecret(); testMultiSign(); testMultiSignQuorumNotMet(); + testMultiSignDelegatorAsSigner(); + testSignForDelegated(); testPermissionValue(all); testTxRequireFeatures(all); testTxDelegableCount(); diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index 4c47914d66..9faa82dfe8 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -1255,7 +1255,9 @@ transactionSignFor( signers.emplaceBack(std::move(signer)); // The array must be sorted and validated. - auto err = sortAndValidateSigners(signers, (*sttx)[sfAccount]); + // For delegated transactions, the delegate account is + // the one forbidden from appearing in its own Signers array. + auto err = sortAndValidateSigners(signers, sttx->getFeePayer()); if (RPC::containsError(err)) return err; } @@ -1422,7 +1424,9 @@ transactionSubmitMultiSigned( } // The array must be sorted and validated. - auto err = sortAndValidateSigners(signers, srcAddressID); + // For delegated transactions, getFeePayer() returns sfDelegate, + // that account is the one forbidden from appearing in its own Signers array. + auto err = sortAndValidateSigners(signers, stTx->getFeePayer()); if (RPC::containsError(err)) return err;