improve(insured-bridge): Settle resolved optimistic price requests that passed dispute period (#3326)

nicholaspai · chrismaree · web-flow · commit 31b60c39d965 · 2021-08-25T09:16:16.000-04:00
* Update BridgePool.sol

* fix tests

* Update BridgePool.sol

* Update BridgePool.sol

* Change settlement logic

* Update packages/core/contracts-ovm/insured-bridge/implementation/BridgePool.sol

Co-authored-by: Chris Maree &lt;christopher.maree@gmail.com&gt;

Co-authored-by: Chris Maree &lt;christopher.maree@gmail.com&gt;
diff --git a/packages/common/src/HardhatConfig.ts b/packages/common/src/HardhatConfig.ts
@@ -53,6 +53,7 @@ export function getHardhatConfig(
         "contracts/financial-templates/expiring-multiparty/Liquidatable.sol": LARGE_CONTRACT_COMPILER_SETTINGS,
         "contracts/oracle/implementation/Voting.sol": LARGE_CONTRACT_COMPILER_SETTINGS,
         "contracts/oracle/implementation/test/VotingTest.sol": LARGE_CONTRACT_COMPILER_SETTINGS,
+        "contracts-ovm/insured-bridge/implementation/BridgePool.sol": LARGE_CONTRACT_COMPILER_SETTINGS,
       },
     },
     ovm: { solcVersion: "0.7.6" },
diff --git a/packages/core/contracts-ovm/insured-bridge/implementation/BridgePool.sol b/packages/core/contracts-ovm/insured-bridge/implementation/BridgePool.sol
@@ -291,8 +291,15 @@ contract BridgePool is Testable, BridgePoolInterface, ExpandedERC20 {
     }
 
     /**
-     * @notice Instantly relay a deposit amount minus fees. Instant relayer earns a reward following the pending relay
-     * challenge period.
+     * @notice Instantly relay a deposit amount minus fees to the recipient. Instant relayer earns a reward following
+     * the pending relay challenge period.
+     * @dev We assume that the caller has performed an off-chain check that the deposit data they are attempting to
+     * relay is valid. If the deposit data is invalid, then the instant relayer has no recourse
+     * to receive their funds back after the invalid deposit data is disputed. Moreover, no one will be able to
+     * resubmit a relay for the invalid deposit data because they know it will get disputed again. On the other hand,
+     * if the deposit data is valid, then even if it is falsely disputed, the instant relayer will eventually get
+     * reimbursed because someone else will be incentivized to resubmit the relay to earn slow relayer rewards. Once the
+     * valid relay is finalized, the instant relayer will be reimbursed.
      * @dev Caller must have approved this contract to spend the deposit amount of L1 tokens to relay. There can only
      * be one instant relayer per relay attempt and disputed relays cannot be sped up.
      * @param _depositData Unique set of L2 deposit data that caller is trying to instantly relay.
@@ -326,25 +333,21 @@ contract BridgePool is Testable, BridgePoolInterface, ExpandedERC20 {
         bytes32 depositHash = _getDepositHash(_depositData);
         RelayData storage relay = relays[depositHash];
 
-        // Note `hasPrice` will return false if liveness has not been passed in the optimistic oracle.
+        // If relay was disputed, then parties in dispute need to go through OptimisticOracle to resolve dispute.
+        require(relays[depositHash].relayState == RelayState.Pending, "Relay not settleable");
+
+        // Attempt to settle OptimisticOracle price as a convenience for the slow relayer who will receive their
+        // dispute bond back. If the price is not settleable, then this call will revert. If the price has already
+        // been settled, then this will not revert and still return the price.
         require(
-            relays[depositHash].relayState == RelayState.Pending &&
-                _getOptimisticOracle().hasPrice(
-                    address(this),
-                    bridgeAdmin.identifier(),
-                    relay.priceRequestTime,
-                    getRelayAncillaryData(_depositData, relay)
-                ),
-            "Relay not settleable"
+            _getOptimisticOracle().settleAndGetPrice(
+                bridgeAdmin.identifier(),
+                relay.priceRequestTime,
+                getRelayAncillaryData(_depositData, relay)
+            ) == int256(1e18), // Canonical value representing "True"; i.e. the proposed relay is valid.
+            "Relay request was not valid"
         );
 
-        // Note: Why don't we have to check the value of the price?
-        // - If the OptimisticOracle has a price and the relayState is PENDING, then we can safely assume that the relay
-        // was validated. This is because this contract proposes a price of 1e18, or "YES" to the identifier posing the
-        // question "Is this relay valid for the associated deposit?". If the proposal is disputed, then the relayState
-        // will be reset to UNINITIALIZED. If the proposal is not disputed, and there is a price available, then the
-        // proposal must have passed the dispute period, assuming the proposal passed optimistic oracle liveness.
-
         // Update the relay state to Finalized. This prevents any re-settling of a relay.
         relay.relayState = RelayState.Finalized;
 
diff --git a/packages/core/test/insured-bridge/BridgePool.js b/packages/core/test/insured-bridge/BridgePool.js
@@ -894,6 +894,16 @@ describe("BridgePool", () => {
         .send({ from: disputer });
 
       assert(await didContractThrow(bridgePool.methods.settleRelay(depositData).send({ from: relayer })));
+
+      // Cannot settle even if disputed price was resolved.
+      const price = toWei("1");
+      const stampedDisputeAncillaryData = await optimisticOracle.methods
+        .stampAncillaryData(relayAncillaryData, bridgePool.options.address)
+        .call();
+      await mockOracle.methods
+        .pushPrice(defaultIdentifier, relayStatus.priceRequestTime.toString(), stampedDisputeAncillaryData, price)
+        .send({ from: owner });
+      assert(await didContractThrow(bridgePool.methods.settleRelay(depositData).send({ from: relayer })));
     });
     it("Can settle pending relays that passed challenge period", async () => {
       // Cache price request timestamp.
@@ -931,41 +941,8 @@ describe("BridgePool", () => {
       );
       assert(await didContractThrow(bridgePool.methods.settleRelay(depositData).send({ from: relayer })));
 
-      // Expire and settle proposal on the OptimisticOracle.
+      // Set time such that optimistic price request is settleable.
       await timer.methods.setCurrentTime(expectedExpirationTimestamp).send({ from: owner });
-      await optimisticOracle.methods
-        .settle(
-          bridgePool.options.address,
-          defaultIdentifier,
-          relayStatus.priceRequestTime.toString(),
-          relayAncillaryData
-        )
-        .send({ from: relayer });
-
-      // Verify price is available.
-      assert.equal(
-        await optimisticOracle.methods
-          .hasPrice(
-            bridgePool.options.address,
-            defaultIdentifier,
-            relayStatus.priceRequestTime.toString(),
-            relayAncillaryData
-          )
-          .call(),
-        true
-      );
-
-      // If the relay status is PENDING and the price is available, then the value must match the original proposal:
-      // 1e18.
-      assert.equal(relayStatus.relayState, InsuredBridgeRelayStateEnum.PENDING);
-      assert.equal(
-        (
-          await optimisticOracle.methods
-            .settleAndGetPrice(defaultIdentifier, relayStatus.priceRequestTime.toString(), relayAncillaryData)
-            .call({ from: bridgePool.options.address })
-        ).toString(),
-        toWei("1")
-      );
 
       // Settle relay and check event logs.
       const settleTxn = await bridgePool.methods.settleRelay(depositData).send({ from: rando });
@@ -1029,22 +1006,13 @@ describe("BridgePool", () => {
           relayData.realizedLpFeePct
         )
         .send({ from: relayer });
-      const relayStatus = await bridgePool.methods.relays(depositHash).call();
 
       // Speed up relay.
       await l1Token.methods.approve(bridgePool.options.address, slowRelayAmountSubFee).send({ from: instantRelayer });
       await bridgePool.methods.speedUpRelay(depositData).send({ from: instantRelayer });
 
-      // Expire and settle proposal on the OptimisticOracle.
+      // Set time such that optimistic price request is settleable.
       await timer.methods.setCurrentTime(expectedExpirationTimestamp).send({ from: owner });
-      await optimisticOracle.methods
-        .settle(
-          bridgePool.options.address,
-          defaultIdentifier,
-          relayStatus.priceRequestTime.toString(),
-          relayAncillaryData
-        )
-        .send({ from: relayer });
 
       const relayerBalanceBefore = await l1Token.methods.balanceOf(relayer).call();
       const instantRelayerBalanceBefore = await l1Token.methods.balanceOf(instantRelayer).call();
@@ -1059,8 +1027,8 @@ describe("BridgePool", () => {
         toBN(await l1Token.methods.balanceOf(relayer).call())
           .sub(toBN(relayerBalanceBefore))
           .toString(),
-        realizedSlowRelayFeeAmount.toString(),
-        "Slow relayer should receive proposal slow relay reward"
+        toBN(totalRelayBond).add(realizedSlowRelayFeeAmount).toString(),
+        "Slow relayer should receive proposal bond + slow relay reward"
       );
       assert.equal(
         toBN(await l1Token.methods.balanceOf(instantRelayer).call())
@@ -1082,6 +1050,42 @@ describe("BridgePool", () => {
         "BridgePool should have balance reduced by relayed amount to recipient"
       );
     });
+    it("Does not revert if price was already settled on OptimisticOracle", async () => {
+      // Cache price request timestamp.
+      const requestTimestamp = (await bridgePool.methods.getCurrentTime().call()).toString();
+      const expectedExpirationTimestamp = (Number(requestTimestamp) + defaultLiveness).toString();
+
+      // Proposer approves pool to withdraw total bond.
+      await l1Token.methods.approve(bridgePool.options.address, totalRelayBond).send({ from: relayer });
+      await bridgePool.methods
+        .relayDeposit(
+          depositData.depositId,
+          depositData.depositTimestamp,
+          depositData.recipient,
+          depositData.l2Sender,
+          depositData.amount,
+          depositData.slowRelayFeePct,
+          depositData.instantRelayFeePct,
+          depositData.quoteTimestamp,
+          relayData.realizedLpFeePct
+        )
+        .send({ from: relayer });
+      const relayStatus = await bridgePool.methods.relays(depositHash).call();
+
+      // Settle price directly via optimistic oracle
+      await timer.methods.setCurrentTime(expectedExpirationTimestamp).send({ from: owner });
+      await optimisticOracle.methods
+        .settle(
+          bridgePool.options.address,
+          defaultIdentifier,
+          relayStatus.priceRequestTime.toString(),
+          relayAncillaryData
+        )
+        .send({ from: rando });
+
+      // Settle relay and check event logs.
+      assert.ok(await bridgePool.methods.settleRelay(depositData).send({ from: rando }), "SettleRelay reverted");
+    });
   });
   describe("Liquidity provision", () => {
     beforeEach(async function () {
@@ -1182,20 +1186,11 @@ describe("BridgePool", () => {
 
       // Next, finalize the bridging action. This should reset the pendingReserves to 0 and increment the utilizedReserves.
       // Expire and settle proposal on the OptimisticOracle.
-      const relayStatus = await bridgePool.methods.relays(depositHash).call();
-
       await timer.methods
         .setCurrentTime(Number(await bridgePool.methods.getCurrentTime().call()) + defaultLiveness)
         .send({ from: owner });
-      await optimisticOracle.methods
-        .settle(
-          bridgePool.options.address,
-          defaultIdentifier,
-          relayStatus.priceRequestTime.toString(),
-          relayAncillaryData
-        )
-        .send({ from: relayer });
       await bridgePool.methods.settleRelay(depositData).send({ from: rando });
+
       // Check the balances have updated correctly. Pending should be 0 (funds are actually utilized now), liquid should
       // be equal to the LP fee of 10 and the utilized should equal the total bridged amount (100).
       assert.equal(await bridgePool.methods.pendingReserves().call(), "0");
@@ -1247,20 +1242,10 @@ describe("BridgePool", () => {
         .send({ from: relayer });
 
       // Expire and settle proposal on the OptimisticOracle.
-      const relayStatus = await bridgePool.methods.relays(depositHash).call();
       await timer.methods.setCurrentTime(expectedExpirationTimestamp).send({ from: owner });
-      await optimisticOracle.methods
-        .settle(
-          bridgePool.options.address,
-          defaultIdentifier,
-          relayStatus.priceRequestTime.toString(),
-          relayAncillaryData
-        )
-        .send({ from: relayer });
+      await bridgePool.methods.settleRelay(depositData).send({ from: rando });
     });
     it("SettleRelay modifies virtual balances", async () => {
-      await bridgePool.methods.settleRelay(depositData).send({ from: rando });
-
       // Exchange rate should still be 1, no fees accumulated yet as no time has passed from the settlement. Pool
       // balance, liquidReserves and utilizedReserves should update accordingly. Calculate the bridge tokens used as
       // the amount sent to the receiver+the bond.
@@ -1282,8 +1267,6 @@ describe("BridgePool", () => {
       );
     });
     it("Fees correctly accumulate to LPs over the one week loan interval", async () => {
-      await bridgePool.methods.settleRelay(depositData).send({ from: rando });
-
       await bridgePool.methods.exchangeRateCurrent().send({ from: rando }); // force sync
 
       // As no time has elapsed, no fees should be earned and the exchange rate should still be 1.
@@ -1340,8 +1323,6 @@ describe("BridgePool", () => {
       assert.equal((await bridgePool.methods.undistributedLpFees().call()).toString(), toWei("2.607616"));
     });
     it("Fees correctly accumulate with multiple relays", async () => {
-      await bridgePool.methods.settleRelay(depositData).send({ from: rando });
-
       // Advance time by a 2 days (172800s) and check that the exchange rate increments accordingly. Expected exchange rate is (910+90+10-(10-0.0000015*172800*10))/1000=1.002592.
       await advanceTime(172800);
 
@@ -1391,18 +1372,9 @@ describe("BridgePool", () => {
       depositHash = soliditySha3(depositDataAbiEncoded);
 
       // Expire and settle proposal on the OptimisticOracle.
-      const relayStatus = await bridgePool.methods.relays(depositHash).call();
       await advanceTime(defaultLiveness);
 
       relayAncillaryData = await bridgePool.methods.getRelayAncillaryData(depositData, relayData).call();
-      await optimisticOracle.methods
-        .settle(
-          bridgePool.options.address,
-          defaultIdentifier,
-          relayStatus.priceRequestTime.toString(),
-          relayAncillaryData
-        )
-        .send({ from: relayer });
 
       // Settle the relay action.
       await bridgePool.methods.settleRelay(depositData).send({ from: rando });
@@ -1477,8 +1449,6 @@ describe("BridgePool", () => {
       assert.equal((await bridgePool.methods.undistributedLpFees().call()).toString(), toWei("0"));
     });
     it("Adding/removing impact exchange rate accumulation as expected", async () => {
-      await bridgePool.methods.settleRelay(depositData).send({ from: rando });
-
       // Advance time by a 2 days (172800s). Exchange rate should be (1000+10*172800*0.0000015)/1000=1.002592
       await advanceTime(172800);
       assert.equal((await bridgePool.methods.exchangeRateCurrent().call()).toString(), toWei("1.002592"));
@@ -1522,9 +1492,8 @@ describe("BridgePool", () => {
     });
     it("Fees & Exchange rate can correctly handel gifted tokens", async () => {
       // We cant control when funds are sent to the contract, just like we cant control when the bridging action
-      // concludes. If someone was to randomly send the contract tokens the exchange rate should ignore this. The contract should ignore the exchange rate if someone was to randomly send tokens.
-
-      await bridgePool.methods.settleRelay(depositData).send({ from: rando });
+      // concludes. If someone was to randomly send the contract tokens the exchange rate should ignore this. The
+      // contract should ignore the exchange rate if someone was to randomly send tokens.
 
       // Advance time by a 2 days (172800s). Exchange rate should be (1000+10*172800*0.0000015)/1000=1.002592
       await advanceTime(172800);
