From de352e1eadb169961ab722496a2aa5bd21663508 Mon Sep 17 00:00:00 2001 From: "Julian M. Rodriguez" <56316686+julianmrodri@users.noreply.github.com> Date: Thu, 19 Oct 2023 11:06:39 -0300 Subject: [PATCH 1/2] TRUST L-7: Restriction on reportViolation (#981) Co-authored-by: Taylor Brent --- contracts/p0/Broker.sol | 2 +- contracts/p1/Broker.sol | 4 +- docs/pause-freeze-states.md | 2 +- test/Broker.test.ts | 22 ------- test/Revenues.test.ts | 122 ++++++++++++++++++++++++++++++++++++ 5 files changed, 126 insertions(+), 26 deletions(-) diff --git a/contracts/p0/Broker.sol b/contracts/p0/Broker.sol index d3e0da204..b53c2e041 100644 --- a/contracts/p0/Broker.sol +++ b/contracts/p0/Broker.sol @@ -92,7 +92,7 @@ contract BrokerP0 is ComponentP0, IBroker { /// Disable the broker until re-enabled by governance /// @custom:protected - function reportViolation() external notTradingPausedOrFrozen { + function reportViolation() external { require(trades[_msgSender()], "unrecognized trade contract"); ITrade trade = ITrade(_msgSender()); TradeKind kind = trade.KIND(); diff --git a/contracts/p1/Broker.sol b/contracts/p1/Broker.sol index 0c233a802..0111d25bc 100644 --- a/contracts/p1/Broker.sol +++ b/contracts/p1/Broker.sol @@ -136,9 +136,9 @@ contract BrokerP1 is ComponentP1, IBroker { /// Disable the broker until re-enabled by governance /// @custom:protected - // checks: not paused (trading), not frozen, caller is a Trade this contract cloned + // checks: caller is a Trade this contract cloned // effects: disabled' = true - function reportViolation() external notTradingPausedOrFrozen { + function reportViolation() external { require(trades[_msgSender()], "unrecognized trade contract"); ITrade trade = ITrade(_msgSender()); TradeKind kind = trade.KIND(); diff --git a/docs/pause-freeze-states.md b/docs/pause-freeze-states.md index 009ab64b2..17b2785fc 100644 --- a/docs/pause-freeze-states.md +++ b/docs/pause-freeze-states.md @@ -17,7 +17,7 @@ A :x: indicates it reverts. | `BackingManager.settleTrade()` | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | | `BasketHandler.refreshBasket()` | :heavy_check_mark: | :x: (unless governance) | :x: (unless governance) | | `Broker.openTrade()` | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | -| `Broker.reportViolation()` | :heavy_check_mark: | :x: | :x: | +| `Broker.reportViolation()` | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | | `Distributor.distribute()` | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | | `Furnace.melt()` | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | | `Main.poke()` | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | diff --git a/test/Broker.test.ts b/test/Broker.test.ts index 90a34a56e..251d0e1a5 100644 --- a/test/Broker.test.ts +++ b/test/Broker.test.ts @@ -574,28 +574,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { // Check nothing changed expect(await broker.batchTradeDisabled()).to.equal(false) }) - - it('Should not allow to report violation if paused or frozen', async () => { - // Check not disabled - expect(await broker.batchTradeDisabled()).to.equal(false) - - await main.connect(owner).pauseTrading() - - await expect(broker.connect(addr1).reportViolation()).to.be.revertedWith( - 'frozen or trading paused' - ) - - await main.connect(owner).unpauseTrading() - - await main.connect(owner).freezeShort() - - await expect(broker.connect(addr1).reportViolation()).to.be.revertedWith( - 'frozen or trading paused' - ) - - // Check nothing changed - expect(await broker.batchTradeDisabled()).to.equal(false) - }) }) describe('Trades', () => { diff --git a/test/Revenues.test.ts b/test/Revenues.test.ts index b8526f139..d631a6f2e 100644 --- a/test/Revenues.test.ts +++ b/test/Revenues.test.ts @@ -2627,11 +2627,133 @@ describe(`Revenues - P${IMPLEMENTATION}`, () => { }, ]) + // Check broker disabled (batch) + expect(await broker.batchTradeDisabled()).to.equal(true) + // Check funds at destinations expect(await rsr.balanceOf(stRSR.address)).to.be.closeTo(minBuyAmt.sub(10), 50) expect(await rToken.balanceOf(furnace.address)).to.be.closeTo(minBuyAmtRToken.sub(10), 50) }) + it('Should report violation even if paused or frozen', async () => { + // This test needs to be in this file and not Broker.test.ts because settleTrade() + // requires the BackingManager _actually_ started the trade + + rewardAmountAAVE = bn('0.5e18') + + // AAVE Rewards + await token2.setRewards(backingManager.address, rewardAmountAAVE) + + // Collect revenue + // Expected values based on Prices between AAVE and RSR/RToken = 1 to 1 (for simplification) + const sellAmt: BigNumber = rewardAmountAAVE.mul(60).div(100) // due to f = 60% + const minBuyAmt: BigNumber = await toMinBuyAmt(sellAmt, fp('1'), fp('1')) + + const sellAmtRToken: BigNumber = rewardAmountAAVE.sub(sellAmt) // Remainder + const minBuyAmtRToken: BigNumber = await toMinBuyAmt(sellAmtRToken, fp('1'), fp('1')) + + // Claim rewards + await facadeTest.claimRewards(rToken.address) + + // Check status of destinations at this point + expect(await rsr.balanceOf(stRSR.address)).to.equal(0) + expect(await rToken.balanceOf(furnace.address)).to.equal(0) + + // Run auctions + await expectEvents(facadeTest.runAuctionsForAllTraders(rToken.address), [ + { + contract: rsrTrader, + name: 'TradeStarted', + args: [anyValue, aaveToken.address, rsr.address, sellAmt, withinQuad(minBuyAmt)], + emitted: true, + }, + { + contract: rTokenTrader, + name: 'TradeStarted', + args: [ + anyValue, + aaveToken.address, + rToken.address, + sellAmtRToken, + withinQuad(minBuyAmtRToken), + ], + emitted: true, + }, + ]) + + // Advance time till auction ended + await advanceTime(config.batchAuctionLength.add(100).toString()) + + // Perform Mock Bids for RSR and RToken (addr1 has balance) + // In order to force deactivation we provide an amount below minBuyAmt, this will represent for our tests an invalid behavior although in a real scenario would retrigger auction + // NOTE: DIFFERENT BEHAVIOR WILL BE OBSERVED ON PRODUCTION GNOSIS AUCTIONS + await rsr.connect(addr1).approve(gnosis.address, minBuyAmt) + await rToken.connect(addr1).approve(gnosis.address, minBuyAmtRToken) + await gnosis.placeBid(0, { + bidder: addr1.address, + sellAmount: sellAmt, + buyAmount: minBuyAmt.sub(10), // Forces in our mock an invalid behavior + }) + await gnosis.placeBid(1, { + bidder: addr1.address, + sellAmount: sellAmtRToken, + buyAmount: minBuyAmtRToken.sub(10), // Forces in our mock an invalid behavior + }) + + // Freeze protocol + await main.connect(owner).freezeShort() + + // Close auctions - Will end trades and also report violation + await expectEvents(facadeTest.runAuctionsForAllTraders(rToken.address), [ + { + contract: broker, + name: 'BatchTradeDisabledSet', + args: [false, true], + emitted: true, + }, + { + contract: rsrTrader, + name: 'TradeSettled', + args: [anyValue, aaveToken.address, rsr.address, sellAmt, minBuyAmt.sub(10)], + emitted: true, + }, + { + contract: rTokenTrader, + name: 'TradeSettled', + args: [ + anyValue, + aaveToken.address, + rToken.address, + sellAmtRToken, + minBuyAmtRToken.sub(10), + ], + emitted: true, + }, + { + contract: rsrTrader, + name: 'TradeStarted', + emitted: false, + }, + { + contract: rTokenTrader, + name: 'TradeStarted', + emitted: false, + }, + ]) + + // Check broker disabled (batch) + expect(await broker.batchTradeDisabled()).to.equal(true) + + // Funds are not distributed if paused or frozen + expect(await rsr.balanceOf(stRSR.address)).to.equal(0) + expect(await rsr.balanceOf(rsrTrader.address)).to.be.closeTo(minBuyAmt.sub(10), 50) + expect(await rToken.balanceOf(furnace.address)).to.equal(0) + expect(await rToken.balanceOf(rTokenTrader.address)).to.be.closeTo( + minBuyAmtRToken.sub(10), + 50 + ) + }) + it('Should not report violation when Dutch Auction clears in geometric phase', async () => { // This test needs to be in this file and not Broker.test.ts because settleTrade() // requires the BackingManager _actually_ started the trade From fcade85cbcd00d7b485453caf2b076ae199c2bdb Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Thu, 19 Oct 2023 10:08:15 -0400 Subject: [PATCH 2/2] Trst-M-11 (#979) --- contracts/plugins/assets/RTokenAsset.sol | 34 +++++++++++------- .../CurveStableRTokenMetapoolCollateral.sol | 5 +++ contracts/plugins/mocks/AssetMock.sol | 9 ++++- .../CrvStableRTokenMetapoolTestSuite.test.ts | 35 +++++++++++++++++++ .../CvxStableRTokenMetapoolTestSuite.test.ts | 35 +++++++++++++++++++ 5 files changed, 104 insertions(+), 14 deletions(-) diff --git a/contracts/plugins/assets/RTokenAsset.sol b/contracts/plugins/assets/RTokenAsset.sol index f187651e3..f82f2ee18 100644 --- a/contracts/plugins/assets/RTokenAsset.sol +++ b/contracts/plugins/assets/RTokenAsset.sol @@ -19,12 +19,14 @@ contract RTokenAsset is IAsset, VersionedAsset, IRTokenOracle { // Component addresses are not mutable in protocol, so it's safe to cache these IMain public immutable main; - IBasketHandler public immutable basketHandler; IAssetRegistry public immutable assetRegistry; IBackingManager public immutable backingManager; + IBasketHandler public immutable basketHandler; IFurnace public immutable furnace; + IERC20 public immutable rsr; + IStRSR public immutable stRSR; - IERC20Metadata public immutable erc20; + IERC20Metadata public immutable erc20; // The RToken uint8 public immutable erc20Decimals; @@ -39,10 +41,12 @@ contract RTokenAsset is IAsset, VersionedAsset, IRTokenOracle { require(maxTradeVolume_ > 0, "invalid max trade volume"); main = erc20_.main(); - basketHandler = main.basketHandler(); assetRegistry = main.assetRegistry(); backingManager = main.backingManager(); + basketHandler = main.basketHandler(); furnace = main.furnace(); + rsr = main.rsr(); + stRSR = main.stRSR(); erc20 = IERC20Metadata(address(erc20_)); erc20Decimals = erc20_.decimals(); @@ -79,18 +83,15 @@ contract RTokenAsset is IAsset, VersionedAsset, IRTokenOracle { assert(low <= high); // not obviously true } - // solhint-disable no-empty-blocks function refresh() public virtual override { // No need to save lastPrice; can piggyback off the backing collateral's saved prices - if (msg.sender != address(assetRegistry)) assetRegistry.refresh(); furnace.melt(); + if (msg.sender != address(assetRegistry)) assetRegistry.refresh(); cachedOracleData.cachedAtTime = 0; // force oracle refresh } - // solhint-enable no-empty-blocks - /// Should not revert /// @dev See `tryPrice` caveat about possible compounding error in calculating price /// @return {UoA/tok} The lower end of the price estimate @@ -130,10 +131,15 @@ contract RTokenAsset is IAsset, VersionedAsset, IRTokenOracle { // solhint-enable no-empty-blocks + /// Force an update to the cache, including refreshing underlying assets + /// @dev Can revert if RToken is unpriced function forceUpdatePrice() external { _updateCachedPrice(); } + /// @dev Can revert if RToken is unpriced + /// @return rTokenPrice {UoA/tok} The mean price estimate + /// @return updatedAt {s} The timestamp of the cache update function latestPrice() external returns (uint192 rTokenPrice, uint256 updatedAt) { // Situations that require an update, from most common to least common. if ( @@ -145,15 +151,17 @@ contract RTokenAsset is IAsset, VersionedAsset, IRTokenOracle { _updateCachedPrice(); } - return (cachedOracleData.cachedPrice, cachedOracleData.cachedAtTime); + rTokenPrice = cachedOracleData.cachedPrice; + updatedAt = cachedOracleData.cachedAtTime; } // ==== Private ==== // Update Oracle Data function _updateCachedPrice() internal { - (uint192 low, uint192 high) = price(); + assetRegistry.refresh(); // will call furnace.melt() + (uint192 low, uint192 high) = price(); require(low != 0 && high != FIX_MAX, "invalid price"); cachedOracleData = CachedOracleData( @@ -183,12 +191,12 @@ contract RTokenAsset is IAsset, VersionedAsset, IRTokenOracle { TradingContext memory ctx; ctx.basketsHeld = basketsHeld; + ctx.ar = assetRegistry; ctx.bm = backingManager; ctx.bh = basketHandler; - ctx.ar = assetRegistry; - ctx.stRSR = main.stRSR(); - ctx.rsr = main.rsr(); - ctx.rToken = main.rToken(); + ctx.rsr = rsr; + ctx.rToken = IRToken(address(erc20)); + ctx.stRSR = stRSR; ctx.minTradeVolume = backingManager.minTradeVolume(); ctx.maxTradeSlippage = backingManager.maxTradeSlippage(); diff --git a/contracts/plugins/assets/curve/CurveStableRTokenMetapoolCollateral.sol b/contracts/plugins/assets/curve/CurveStableRTokenMetapoolCollateral.sol index 420e002f4..780a083a8 100644 --- a/contracts/plugins/assets/curve/CurveStableRTokenMetapoolCollateral.sol +++ b/contracts/plugins/assets/curve/CurveStableRTokenMetapoolCollateral.sol @@ -42,6 +42,11 @@ contract CurveStableRTokenMetapoolCollateral is CurveStableMetapoolCollateral { pairedAssetRegistry = IRToken(address(pairedToken)).main().assetRegistry(); } + function refresh() public override { + pairedAssetRegistry.refresh(); // refresh all registered assets + super.refresh(); // already handles all necessary default checks + } + /// Can revert, used by `_anyDepeggedOutsidePool()` /// Should not return FIX_MAX for low /// @return lowPaired {UoA/pairedTok} The low price estimate of the paired token diff --git a/contracts/plugins/mocks/AssetMock.sol b/contracts/plugins/mocks/AssetMock.sol index c1b495380..0396a5ea3 100644 --- a/contracts/plugins/mocks/AssetMock.sol +++ b/contracts/plugins/mocks/AssetMock.sol @@ -4,6 +4,8 @@ pragma solidity 0.8.19; import "../assets/Asset.sol"; contract AssetMock is Asset { + bool public stale; + uint192 private lowPrice; uint192 private highPrice; @@ -40,13 +42,18 @@ contract AssetMock is Asset { uint192 ) { + require(!stale, "stale price"); return (lowPrice, highPrice, 0); } /// Should not revert /// Refresh saved prices function refresh() public virtual override { - // pass + stale = false; + } + + function setStale(bool _stale) external { + stale = _stale; } function setPrice(uint192 low, uint192 high) external { diff --git a/test/plugins/individual-collateral/curve/crv/CrvStableRTokenMetapoolTestSuite.test.ts b/test/plugins/individual-collateral/curve/crv/CrvStableRTokenMetapoolTestSuite.test.ts index a97702d5a..fd62e8ee7 100644 --- a/test/plugins/individual-collateral/curve/crv/CrvStableRTokenMetapoolTestSuite.test.ts +++ b/test/plugins/individual-collateral/curve/crv/CrvStableRTokenMetapoolTestSuite.test.ts @@ -241,6 +241,41 @@ const collateralSpecificStatusTests = () => { // refresh() should not revert await collateral.refresh() }) + + it('Regression test -- refreshes inner RTokenAsset on refresh()', async () => { + const [collateral] = await deployCollateral({}) + const initialPrice = await collateral.price() + expect(initialPrice[0]).to.be.gt(0) + expect(initialPrice[1]).to.be.lt(MAX_UINT192) + + // Swap out eUSD's RTokenAsset with a mock one + const AssetMockFactory = await ethers.getContractFactory('AssetMock') + const mockRTokenAsset = await AssetMockFactory.deploy( + bn('1'), // unused + ONE_ADDRESS, // unused + bn('1'), // unused + eUSD, + bn('1'), // unused + bn('1') // unused + ) + const eUSDAssetRegistry = await ethers.getContractAt( + 'IAssetRegistry', + '0x9B85aC04A09c8C813c37de9B3d563C2D3F936162' + ) + await whileImpersonating('0xc8Ee187A5e5c9dC9b42414Ddf861FFc615446a2c', async (signer) => { + await eUSDAssetRegistry.connect(signer).swapRegistered(mockRTokenAsset.address) + }) + + // Set RTokenAsset price to stale + await mockRTokenAsset.setStale(true) + expect(await mockRTokenAsset.stale()).to.be.true + + // Refresh CurveStableRTokenMetapoolCollateral + await collateral.refresh() + + // Stale should be false again + expect(await mockRTokenAsset.stale()).to.be.false + }) } /* diff --git a/test/plugins/individual-collateral/curve/cvx/CvxStableRTokenMetapoolTestSuite.test.ts b/test/plugins/individual-collateral/curve/cvx/CvxStableRTokenMetapoolTestSuite.test.ts index bfb1f3018..ab50ef36a 100644 --- a/test/plugins/individual-collateral/curve/cvx/CvxStableRTokenMetapoolTestSuite.test.ts +++ b/test/plugins/individual-collateral/curve/cvx/CvxStableRTokenMetapoolTestSuite.test.ts @@ -243,6 +243,41 @@ const collateralSpecificStatusTests = () => { // refresh() should not revert await collateral.refresh() }) + + it('Regression test -- refreshes inner RTokenAsset on refresh()', async () => { + const [collateral] = await deployCollateral({}) + const initialPrice = await collateral.price() + expect(initialPrice[0]).to.be.gt(0) + expect(initialPrice[1]).to.be.lt(MAX_UINT192) + + // Swap out eUSD's RTokenAsset with a mock one + const AssetMockFactory = await ethers.getContractFactory('AssetMock') + const mockRTokenAsset = await AssetMockFactory.deploy( + bn('1'), // unused + ONE_ADDRESS, // unused + bn('1'), // unused + eUSD, + bn('1'), // unused + bn('1') // unused + ) + const eUSDAssetRegistry = await ethers.getContractAt( + 'IAssetRegistry', + '0x9B85aC04A09c8C813c37de9B3d563C2D3F936162' + ) + await whileImpersonating('0xc8Ee187A5e5c9dC9b42414Ddf861FFc615446a2c', async (signer) => { + await eUSDAssetRegistry.connect(signer).swapRegistered(mockRTokenAsset.address) + }) + + // Set RTokenAsset price to stale + await mockRTokenAsset.setStale(true) + expect(await mockRTokenAsset.stale()).to.be.true + + // Refresh CurveStableRTokenMetapoolCollateral + await collateral.refresh() + + // Stale should be false again + expect(await mockRTokenAsset.stale()).to.be.false + }) } /*