Skip to content

Commit

Permalink
[fold] Disallow the same currency with different issuers:
Browse files Browse the repository at this point in the history 
If the bridge locked USD and also issued USD it's possible to break
sidechain invariants. This patch disallows a bridge that locks or issues
the same currency more than once (even if they have different issuers)

This commit also removes the constraint that the same bridge can't exist
on the same chain. Now that bridge hash only include the currency and
account this constraint would have disallowed far too many bridges.
  • Loading branch information
@seelabs
seelabs committed Aug 25, 2023
1 parent 6204914 commit 9c4abe0
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 58 deletions.
13 changes: 4 additions & 9 deletions src/ripple/app/tx/impl/XChainBridge.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1453,19 +1453,14 @@ XChainCreateBridge::preclaim(PreclaimContext const& ctx)
auto const account = ctx.tx[sfAccount];
auto const bridgeSpec = ctx.tx[sfXChainBridge];

// The bridge can't already exist on this ledger, and the bridge for the
// locking chain and issuing chain can't live on the same ledger.
if (ctx.view.read(
keylet::bridge(bridgeSpec, STXChainBridge::ChainType::locking)) ||
ctx.view.read(
keylet::bridge(bridgeSpec, STXChainBridge::ChainType::issuing)))
STXChainBridge::ChainType const chainType =
STXChainBridge::srcChain(account == bridgeSpec.lockingChainDoor());

if (ctx.view.read(keylet::bridge(bridgeSpec, chainType)))
{
return tecDUPLICATE;
}

STXChainBridge::ChainType const chainType =
STXChainBridge::srcChain(account == bridgeSpec.lockingChainDoor());

if (!isXRP(bridgeSpec.issue(chainType)))
{
auto const sleIssuer =
Expand Down
9 changes: 3 additions & 6 deletions src/ripple/protocol/impl/Indexes.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,16 +400,13 @@ Keylet
bridge(STXChainBridge const& bridge, STXChainBridge::ChainType chainType)
{
// A door account can support multiple bridges. On the locking chain
// there can only be one bridge per lockingChainIssue. On the issuing chain
// there can only be one bridge per issuingChainIssue.
// there can only be one bridge per lockingChainCurrency. On the issuing
// chain there can only be one bridge per issuingChainCurrency.
auto const& issue = bridge.issue(chainType);
return {
ltBRIDGE,
indexHash(
LedgerNameSpace::BRIDGE,
bridge.door(chainType),
issue.account,
issue.currency)};
LedgerNameSpace::BRIDGE, bridge.door(chainType), issue.currency)};
}

Keylet
Expand Down
19 changes: 5 additions & 14 deletions src/ripple/rpc/handlers/LedgerEntry.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,22 +407,13 @@ doLedgerEntry(RPC::JsonContext& context)
// block has a larger scope so the `bridge` variable doesn't
// need to be an optional.
STXChainBridge const bridge(context.params[jss::bridge]);

std::optional<STXChainBridge::ChainType> chainType;
for (auto const ct :
{STXChainBridge::ChainType::locking,
STXChainBridge::ChainType::issuing})
{
if (*account == bridge.door(ct))
{
chainType.emplace(ct);
break;
}
}
if (!chainType)
STXChainBridge::ChainType const chainType =
STXChainBridge::srcChain(
account == bridge.lockingChainDoor());
if (account != bridge.door(chainType))
return std::nullopt;

return keylet::bridge(bridge, *chainType);
return keylet::bridge(bridge, chainType);
}
catch (...)
{
Expand Down
90 changes: 61 additions & 29 deletions src/test/app/XChain_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,23 +554,38 @@ struct XChain_test : public beast::unit_test::suite,
}

void
testXChainOneBridgePerAccount()
testXChainBridgeCreateConstraints()
{
/**
* One bridge per door account tests. Some related cases are
* tested previously.
* Bridge create constraints tests.
*
* Define the door's bridge asset collection as the collection of all
* the issuing assets for which the door account is on the issuing chain
* and all the locking assets for which the door account is on the
* locking chain. (note: a door account can simultaneously be on an
* issuing and locking chain). A new bridge is not a duplicate as long
* as the new bridge asset collection does not contain any duplicate
* currencies (even if the issuers differ).
*
* Create bridges:
* Note we only test one side, since one side does not know what
* bridges the other side has. We pick the sidechain.
* | bridge spec | result
* | main side |
* case 1 | A -> B | tesSUCCESS
* case 2 | B <- A | tecDUPLICATE
* case 3 | B -> A | tecDUPLICATE
* case 4 | C <- B | tecDUPLICATE
* case 5 | C <- A | tesSUCCESS
* where A -> B, means A is the locking door and B is the issuing door.
*
*| Owner | Locking | Issuing | Comment |
*| a1 | a1 USD/GW | USD/B | |
*| a2 | a2 USD/GW | USD/B | Same locking & issuing assets |
*| | | | |
*| a3 | a3 USD/GW | USD/a4 | |
*| a4 | a4 USD/GW | USD/a4 | Same bridge, different accounts |
*| | | | |
*| B | A USD/GW | USD/B | |
*| B | A EUR/GW | USD/B | Fail: Same issuing asset |
*| | | | |
*| A | A USD/B | USD/C | |
*| A | A USD/B | EUR/B | Fail: Same locking asset |
*| A | A USD/C | EUR/B | Fail: Same locking asset currency |
*| | | | |
*| A | A USD/GW | USD/B | Fail: Same bridge not allowed |
*| A | B USD/GW | USD/A | Fail: "A" has USD already |
*| B | A EUR/GW | USD/B | Fail: |
*
* Note that, now from sidechain's point of view, A is both
* a local locking door and a foreign locking door on different
Expand All @@ -587,7 +602,7 @@ struct XChain_test : public beast::unit_test::suite,
*/

using namespace jtx;
testcase("One bridge per account");
testcase("Bridge create constraints");
XEnv env(*this, true);
auto& A = scAlice;
auto& B = scBob;
Expand All @@ -596,29 +611,44 @@ struct XChain_test : public beast::unit_test::suite,
auto BUSD = B["USD"];
auto CUSD = C["USD"];
auto GUSD = scGw["USD"];
auto AEUR = A["EUR"];
auto BEUR = B["EUR"];
auto CEUR = C["EUR"];
auto GEUR = scGw["EUR"];

// Accounts to own single brdiges
Account const a1("a1");
Account const a2("a2");
Account const a3("a3");
Account const a4("a4");

env.fund(XRP(10000), a1, a2, a3, a4);
env.close();

// Add a bridge on two different accounts with the same locking and
// issuing assets
env.tx(create_bridge(a1, bridge(a1, GUSD, B, BUSD))).close();
env.tx(create_bridge(a2, bridge(a2, GUSD, B, BUSD))).close();

// Add the exact same bridge to two different accoutns (one must locking
// account and one must be issuing)
env.tx(create_bridge(a3, bridge(a3, GUSD, a4, a4["USD"]))).close();
env.tx(create_bridge(a4, bridge(a3, GUSD, a4, a4["USD"]))).close();

// Test case 1 ~ 5, create bridges
// Issuing doors use USDs issued by them in bridge spec.
// Define the door's bridge asset collecion as the collection of all the
// issuing assets for which the door account is on the issuing chain and
// all the locking assets for which the door account is on the locking
// chain. (note: a door account can simultainious be on an issuing and
// locking chain). A new bridge is not a duplicate as long as the new
// bridge asset collection does not contain any dupicates.
auto const goodBridge1 = bridge(A, GUSD, B, BUSD);
auto const goodBridge2 = bridge(A, BUSD, C, CUSD);
env.tx(create_bridge(B, goodBridge1)).close();
// A bridge must be unique to the chain, even if it has different door
// accounts (otherwise a bridge spec isn't enough to look up a bridge)
env.tx(create_bridge(A, bridge(A, GUSD, B, BUSD)), ter(tecDUPLICATE))
.close();
env.tx(create_bridge(A, bridge(B, GUSD, A, AUSD))).close();
// Issuing asset is the same, this is a duplicate
env.tx(create_bridge(A, bridge(B, CUSD, A, AUSD)), ter(tecDUPLICATE))
env.tx(create_bridge(B, bridge(A, GEUR, B, BUSD)), ter(tecDUPLICATE))
.close();
env.tx(create_bridge(A, goodBridge2), ter(tesSUCCESS)).close();
// Locking asset is the same - this is a duplicate
env.tx(create_bridge(A, bridge(A, BUSD, B, BUSD)), ter(tecDUPLICATE))
env.tx(create_bridge(A, bridge(A, BUSD, B, BEUR)), ter(tecDUPLICATE))
.close();
// Locking asset is USD - this is a duplicate even tho it has a
// different issuer
env.tx(create_bridge(A, bridge(A, CUSD, B, BEUR)), ter(tecDUPLICATE))
.close();

// Test case 6 and 7, commits
Expand Down Expand Up @@ -4169,9 +4199,11 @@ struct XChain_test : public beast::unit_test::suite,
void
run() override
{
testXChainBridgeCreateConstraints();
return;
testXChainBridgeExtraFields();
testXChainCreateBridge();
testXChainOneBridgePerAccount();
testXChainBridgeCreateConstraints();
testXChainCreateBridgeMatrix();
testXChainModifyBridge();
testXChainCreateClaimID();
Expand Down

0 comments on commit 9c4abe0

Please sign in to comment.