From 2f5f122a75f1d10e7e5fe9955538a79fab1a6aa1 Mon Sep 17 00:00:00 2001 From: Richard Holland Date: Tue, 29 Mar 2022 11:34:29 +0000 Subject: [PATCH] bug fixes for IOU escrows --- src/ripple/app/tx/impl/Escrow.cpp | 109 +++++++++++----------- src/ripple/app/tx/impl/InvariantCheck.cpp | 11 +-- 2 files changed, 59 insertions(+), 61 deletions(-) diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index 1051fd71b..80bc210e0 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -582,7 +582,7 @@ EscrowFinish::doApply() auto const sle = ctx_.view().peek(keylet::account(account)); - auto amount = slep->getFieldAmount(sfBalance); + auto amount = slep->getFieldAmount(sfAmount); if (isXRP(amount)) (*sled)[sfBalance] = (*sled)[sfBalance] + (*slep)[sfAmount]; @@ -654,6 +654,10 @@ EscrowFinish::doApply() return tefINTERNAL; } + // dstLow XNOR srcLow tells us if we need to flip the balance amount + // on the destination line + bool flipDstAmt = !((dstLow && srcLow) || (!dstLow && !srcLow)); + // check if the dest line exists, and if it doesn't create it if we're allowed to if (!sleDstLine) { @@ -684,7 +688,7 @@ EscrowFinish::doApply() false, // authorize account (sleDstAcc->getFlags() & lsfDefaultRipple) == 0, false, // freeze trust line - amount, // initial balance + flipDstAmt ? -amount : amount, // initial balance Issue(currency, account_), // limit of zero 0, // quality in 0, // quality out @@ -706,42 +710,7 @@ EscrowFinish::doApply() << "Finishing an escrow for destination frozen trustline."; return tecFROZEN; } - - // decrement source balance - { - STAmount priorBalance = - srcLow ? (*sleSrcLine)[sfBalance] : -((*sleSrcLine)[sfBalance]); - - STAmount finalBalance = priorBalance - amount; - if (finalBalance < beast::zero) - { - JLOG(j.warn()) - << "Escrow finish results in a negative balance on source line"; - return tecINTERNAL; - } - - if (!sleSrcLine->isFieldPresent(sfLockedBalance)) - { - JLOG(j.warn()) - << "Escrow finish could not find sfLockedBalance on source line"; - return tecINTERNAL; - } - - STAmount priorLockedBalance = - srcLow ? (*sleSrcLine)[sfLockedBalance] : -((*sleSrcLine)[sfLockedBalance]); - - STAmount finalLockedBalance = priorLockedBalance - amount; - if (finalLockedBalance < beast::zero) - { - JLOG(j.warn()) - << "Escrow finish results in a negative locked balance on source line"; - return tecINTERNAL; - } - - sleSrcLine->setFieldAmount(sfBalance, srcLow ? finalBalance : -finalBalance); - sleSrcLine->setFieldAmount(sfLockedBalance, srcLow ? finalLockedBalance : -finalLockedBalance); - } - + // increment dest balance { // if balance is higher than limit then only allow if dest is signer @@ -751,7 +720,7 @@ EscrowFinish::doApply() STAmount priorBalance = dstLow ? (*sleDstLine)[sfBalance] : -((*sleDstLine)[sfBalance]); - STAmount finalBalance = priorBalance + amount; + STAmount finalBalance = priorBalance + (flipDstAmt ? -amount : amount); if (finalBalance < priorBalance) { @@ -770,9 +739,52 @@ EscrowFinish::doApply() sleDstLine->setFieldAmount(sfBalance, dstLow ? finalBalance : -finalBalance); } - view.update(sleSrcLine); - view.update(sleDstLine); } + + // decrement source balance + { + STAmount priorBalance = + srcLow ? (*sleSrcLine)[sfBalance] : -((*sleSrcLine)[sfBalance]); + + STAmount finalBalance = priorBalance - amount; + if (finalBalance < beast::zero) + { + JLOG(j.warn()) + << "Escrow finish results in a negative balance on source line"; + return tecINTERNAL; + } + + if (!sleSrcLine->isFieldPresent(sfLockedBalance)) + { + JLOG(j.warn()) + << "Escrow finish could not find sfLockedBalance on source line"; + return tecINTERNAL; + } + + STAmount priorLockedBalance = + srcLow ? (*sleSrcLine)[sfLockedBalance] : -((*sleSrcLine)[sfLockedBalance]); + + STAmount finalLockedBalance = priorLockedBalance - amount; + if (finalLockedBalance < beast::zero) + { + JLOG(j.warn()) + << "Escrow finish results in a negative locked balance on source line"; + return tecINTERNAL; + } + + sleSrcLine->setFieldAmount(sfBalance, srcLow ? finalBalance : -finalBalance); + + if (finalLockedBalance == beast::zero) + sleSrcLine->makeFieldAbsent(sfLockedBalance); + else + sleSrcLine->setFieldAmount(sfLockedBalance, srcLow ? finalLockedBalance : -finalLockedBalance); + } + + // update source and dest lines to reflect balance mutation + view.update(sleSrcLine); + + if (sleDstLine) + view.update(sleDstLine); } else return tecINTERNAL; // should never happen @@ -861,7 +873,7 @@ EscrowCancel::doApply() } auto const sle = ctx_.view().peek(keylet::account(account)); - auto amount = slep->getFieldAmount(sfBalance); + auto amount = slep->getFieldAmount(sfAmount); // Transfer amount back to the owner (or unlock it in TL case) if (isXRP(amount)) @@ -886,19 +898,6 @@ EscrowCancel::doApply() return tecINTERNAL; } - STAmount priorBalance = - isLow ? (*sleSrcLine)[sfBalance] : -(*sleSrcLine)[sfBalance]; - - STAmount finalBalance = priorBalance - amount; - if (finalBalance < beast::zero) - { - JLOG(j_.warn()) - << "Escrow cancel would force line balance below zero"; - return tecINTERNAL; - } - - sleSrcLine->setFieldAmount(sfBalance, isLow ? finalBalance : -finalBalance); - if (finalLockedBalance == beast::zero) sleSrcLine->makeFieldAbsent(sfLockedBalance); else diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 9d158678a..6f7140453 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -293,15 +293,14 @@ NoZeroEscrow::finalize( // bypass this invariant check for IOU escrows if (bad_ && rv.rules().enabled(featurePaychanAndEscrowForTokens) && - txn.isFieldPresent(sfTransactionType) && - txn.isFieldPresent(sfAmount)) + txn.isFieldPresent(sfTransactionType)) { uint16_t tt = txn.getFieldU16(sfTransactionType); - if (tt == ttESCROW_CREATE || tt == ttESCROW_FINISH || tt == ttESCROW_CANCEL) - { - if (!isXRP(txn.getFieldAmount(sfAmount))) + if (tt == ttESCROW_CANCEL || tt == ttESCROW_FINISH) + return true; + + if (txn.isFieldPresent(sfAmount) && !isXRP(txn.getFieldAmount(sfAmount))) return true; - } } if (bad_)