Skip to content

fix: Fix Audit/Attackathon reports (Phase 2) #7537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gregtatcam wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix: Fix Audit/Attackathon reports (Phase 2) #7537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions src/libxrpl/tx/transactors/check/CheckCash.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
#include <xrpl/protocol/Issue.h>
#include <xrpl/protocol/Keylet.h>
#include <xrpl/protocol/LedgerFormats.h>
#include <xrpl/protocol/MPTAmount.h>
#include <xrpl/protocol/MPTIssue.h>
#include <xrpl/protocol/Protocol.h>
#include <xrpl/protocol/Quality.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/STLedgerEntry.h>
Expand Down Expand Up @@ -368,18 +369,29 @@ CheckCash::doApply()
else
{
// Note that for DeliverMin we don't know exactly how much
// currency we want flow to deliver. We can't ask for the
// maximum possible currency because there might be a gateway
// transfer rate to account for. Since the transfer rate cannot
// exceed 200%, we use 1/2 maxValue as our limit.
// currency we want flow to deliver. For IOUs, use a value
// higher than any real delivery as the request. MPTs are
// bounded integral amounts, so use the maximum output the check
// can actually deliver without exceeding SendMax.
auto const maxDeliverMin = [&]() {
return optDeliverMin->asset().visit(
[&](Issue const&) {
return STAmount(
optDeliverMin->asset(), STAmount::kMaxValue / 2, STAmount::kMaxOffset);
},
[&](MPTIssue const&) {
return STAmount(optDeliverMin->asset(), kMaxMpTokenAmount / 2);
[&](MPTIssue const& issue) {
MPTAmount maxDeliver = sendMax.mpt();
auto const& issuer = issue.getIssuer();
if (srcId != issuer && accountID_ != issuer)
{
auto const rate = transferRate(psb, issue.getMptID());
// Request at most floor(SendMax / rate). The endpoint reverse pass
// will quote ceil(output * rate), so this keeps the input
// representable and within SendMax.
maxDeliver =
mulRatio(maxDeliver, QUALITY_ONE, rate.value, /*roundUp*/ false);
}
return STAmount(maxDeliver, issue);
});
};
STAmount const flowDeliver{
Expand Down
53 changes: 53 additions & 0 deletions src/test/app/CheckMPT_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <xrpl/protocol/Issue.h>
#include <xrpl/protocol/KeyType.h>
#include <xrpl/protocol/LedgerFormats.h>
#include <xrpl/protocol/Protocol.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/TER.h>
Expand Down Expand Up @@ -653,6 +654,32 @@ class CheckMPT_test : public beast::unit_test::Suite
BEAST_EXPECT(ownerCount(env, alice) == 1);
BEAST_EXPECT(ownerCount(env, bob) == 1);
}

{
Env env{*this, features};

env.fund(XRP(1'000), gw, alice, bob);

// MPT DeliverMin should not be capped at half of the legal range.
std::uint64_t constexpr deliverMin = (kMaxMpTokenAmount / 2) + 1;
MPT const usd = MPTTester(
{.env = env, .issuer = gw, .holders = {alice, bob}, .maxAmt = kMaxMpTokenAmount});

env(pay(gw, alice, usd(deliverMin)));
env.close();

uint256 const chkId{getCheckIndex(alice, env.seq(alice))};
env(check::create(alice, bob, usd(deliverMin)));
env.close();

env(check::cash(bob, chkId, check::DeliverMin(usd(deliverMin))));
verifyDeliveredAmount(env, usd(deliverMin));
env.require(Balance(alice, usd(0)));
env.require(Balance(bob, usd(deliverMin)));
BEAST_EXPECT(checksOnAccount(env, alice).empty());
BEAST_EXPECT(checksOnAccount(env, bob).empty());
}

{
// Examine the effects of the asfRequireAuth flag.
Env env(*this, features);
Expand Down Expand Up @@ -831,6 +858,32 @@ class CheckMPT_test : public beast::unit_test::Suite
BEAST_EXPECT(checksOnAccount(env, alice).size() == 0);
BEAST_EXPECT(checksOnAccount(env, bob).size() == 0);
#endif

// With the maximum transfer fee, this is the largest output whose
// fee-adjusted debit is still within SendMax.
std::uint64_t constexpr maxDeliver = (kMaxMpTokenAmount / 3) * 2;
MPT const eur = MPTTester(
{.env = env,
.issuer = gw,
.holders = {alice, bob},
.transferFee = kMaxTransferFee,
.maxAmt = kMaxMpTokenAmount});

env(pay(gw, alice, eur(kMaxMpTokenAmount)));
env.close();

uint256 const chkIdMax{getCheckIndex(alice, env.seq(alice))};
env(check::create(alice, bob, eur(kMaxMpTokenAmount)));
env.close();

// The DeliverMin cap must divide SendMax by the rate before flow()
// computes the fee-adjusted input.
env(check::cash(bob, chkIdMax, check::DeliverMin(eur(maxDeliver))));
verifyDeliveredAmount(env, eur(maxDeliver));
env.require(Balance(alice, eur(1)));
env.require(Balance(bob, eur(maxDeliver)));
BEAST_EXPECT(checksOnAccount(env, alice).empty());
BEAST_EXPECT(checksOnAccount(env, bob).empty());
}

void
Expand Down
Loading