Bunch of small changes (#877)

contracts/p0/AssetRegistry.sol

@@ -144,6 +144,13 @@ contract AssetRegistryP0 is ComponentP0, IAssetRegistry {
 
     /// Register an asset, unregistering any previous asset with the same ERC20.
     function _registerIgnoringCollisions(IAsset asset) private returns (bool swapped) {
+        if (asset.isCollateral()) {
+            require(
+                ICollateral(address(asset)).status() == CollateralStatus.SOUND,
+                "collateral not sound"
+            );
+        }
+
         if (_erc20s.contains(address(asset.erc20())) && assets[asset.erc20()] == asset)
             return false;
 

contracts/p0/Broker.sol

@@ -4,6 +4,7 @@ pragma solidity 0.8.19;
 import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
 import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import "@openzeppelin/contracts/proxy/Clones.sol";
 import "../plugins/trading/DutchTrade.sol";
 import "../plugins/trading/GnosisTrade.sol";
 import "../interfaces/IBroker.sol";
@@ -174,7 +175,7 @@ contract BrokerP0 is ComponentP0, IBroker {
 
     function newBatchAuction(TradeRequest memory req, address caller) private returns (ITrade) {
         require(batchAuctionLength > 0, "batch auctions not enabled");
-        GnosisTrade trade = new GnosisTrade();
+        GnosisTrade trade = GnosisTrade(Clones.clone(address(batchTradeImplementation)));
         trades[address(trade)] = true;
 
         // Apply Gnosis EasyAuction-specific resizing of req, if needed: Ensure that
@@ -202,7 +203,7 @@ contract BrokerP0 is ComponentP0, IBroker {
         ITrading caller
     ) private returns (ITrade) {
         require(dutchAuctionLength > 0, "dutch auctions not enabled");
-        DutchTrade trade = new DutchTrade();
+        DutchTrade trade = DutchTrade(Clones.clone(address(dutchTradeImplementation)));
         trades[address(trade)] = true;
 
         IERC20Metadata(address(req.sell.erc20())).safeTransferFrom(

contracts/p0/Deployer.sol

@@ -4,6 +4,8 @@ pragma solidity 0.8.19;
 import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
 import "../plugins/assets/Asset.sol";
 import "../plugins/assets/RTokenAsset.sol";
+import "../plugins/trading/DutchTrade.sol";
+import "../plugins/trading/GnosisTrade.sol";
 import "./AssetRegistry.sol";
 import "./BackingManager.sol";
 import "./BasketHandler.sol";
@@ -113,9 +115,9 @@ contract DeployerP0 is IDeployer, Versioned {
         main.broker().init(
             main,
             gnosis,
-            ITrade(address(1)),
+            ITrade(address(new GnosisTrade())),
             params.batchAuctionLength,
-            ITrade(address(1)),
+            ITrade(address(new DutchTrade())),
             params.dutchAuctionLength
         );
 

contracts/p1/AssetRegistry.sol

@@ -98,6 +98,8 @@ contract AssetRegistryP1 is ComponentP1, IAssetRegistry {
     }
 
     /// Unregister an asset, requiring that it is already registered
+    /// Rewards are NOT claimed by default when unregistering due to security concerns.
+    /// If the collateral is secure, governance should claim rewards before unregistering.
     /// @custom:governance
     // checks: assets[asset.erc20()] == asset
     // effects: assets' = assets - {asset.erc20():_} + {asset.erc20(), asset}
@@ -186,6 +188,13 @@ contract AssetRegistryP1 is ComponentP1, IAssetRegistry {
     // effects: assets' = assets.set(asset.erc20(), asset)
     // returns: assets[asset.erc20()] != asset
     function _registerIgnoringCollisions(IAsset asset) private returns (bool swapped) {
+        if (asset.isCollateral()) {
+            require(
+                ICollateral(address(asset)).status() == CollateralStatus.SOUND,
+                "collateral not sound"
+            );
+        }
+
         IERC20Metadata erc20 = asset.erc20();
         if (_erc20s.contains(address(erc20))) {
             if (assets[erc20] == asset) return false;

contracts/p1/BackingManager.sol

@@ -82,6 +82,10 @@ contract BackingManagerP1 is TradingP1, IBackingManager {
 
     /// Settle a single trade. If the caller is the trade, try chaining into rebalance()
     /// While this function is not nonReentrant, its two subsets each individually are
+    /// If the caller is a trade contract, initiate the next trade.
+    /// This is done in order to better align incentives,
+    /// and have the last bidder be the one to start the next auction.
+    /// This behaviour currently only happens for Dutch Trade.
     /// @param sell The sell token in the trade
     /// @return trade The ITrade contract settled
     /// @custom:interaction

contracts/p1/mixins/Trading.sol

@@ -117,7 +117,6 @@ abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeabl
         TradeRequest memory req,
         TradePrices memory prices
     ) internal returns (ITrade trade) {
-        /*  */
         IERC20 sell = req.sell.erc20();
         assert(address(trades[sell]) == address(0));
 

contracts/plugins/mocks/InvalidRefPerTokCollateral.sol

@@ -18,9 +18,10 @@ contract InvalidRefPerTokCollateralMock is AppreciatingFiatCollateral {
 
     // solhint-disable no-empty-blocks
 
-    constructor(CollateralConfig memory config, uint192 revenueHiding)
-        AppreciatingFiatCollateral(config, revenueHiding)
-    {}
+    constructor(
+        CollateralConfig memory config,
+        uint192 revenueHiding
+    ) AppreciatingFiatCollateral(config, revenueHiding) {}
 
     // solhint-enable no-empty-blocks
 
@@ -68,6 +69,11 @@ contract InvalidRefPerTokCollateralMock is AppreciatingFiatCollateral {
         refPerTokRevert = on;
     }
 
+    // Setter for status
+    function setStatus(CollateralStatus _status) external {
+        markStatus(_status);
+    }
+
     function refPerTok() public view virtual override returns (uint192) {
         if (refPerTokRevert) revert(); // Revert with no reason
         return rateMock;

contracts/plugins/trading/DutchTrade.sol

@@ -98,6 +98,8 @@ contract DutchTrade is ITrade {
 
     constructor() {
         ONE_BLOCK = NetworkConfigLib.blocktime();
+
+        status = TradeStatus.CLOSED;
     }
 
     // === External ===

contracts/plugins/trading/GnosisTrade.sol

@@ -58,6 +58,10 @@ contract GnosisTrade is ITrade {
         status = end;
     }
 
+    constructor() {
+        status = TradeStatus.CLOSED;
+    }
+
     /// Constructor function, can only be called once
     /// @dev Expects sell tokens to already be present
     /// @custom:interaction reentrancy-safe b/c state-locking

docs/system-design.md

@@ -33,9 +33,8 @@ Some of the core contracts in our system regularly own ERC20 tokens. In each cas
 
 ### RToken Lifecycle
 
-1. During SlowIssuance, the `RToken` transfers collateral tokens from the caller's address into itself.
-2. At vesting time, the `RToken` contract mints new RToken to the recipient and transfers the held collateral to the `BackingManager`. If the `BasketHandler` has updated the basket since issuance began, then the collateral is instead returned to the recipient and no RToken is minted.
-3. During redemption, RToken is burnt from the redeemer's account and they are transferred a prorata share of backing collateral from the `BackingManager`.
+1. During minting, the `RToken` transfers collateral tokens from the caller's address into itself and mints new RToken to the caller's address. Minting amount must be less than the current throttle limit, or the transaction will revert.
+2. During redemption, RToken is burnt from the redeemer's account and they are transferred a prorata share of backing collateral from the `BackingManager`.
 
 ## Protocol Assumptions
 

test/Broker.test.ts

@@ -1,4 +1,4 @@
-import { loadFixture } from '@nomicfoundation/hardhat-network-helpers'
+import { loadFixture, setStorageAt } from '@nomicfoundation/hardhat-network-helpers'
 import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
 import { anyValue } from '@nomicfoundation/hardhat-chai-matchers/withArgs'
 import { expect } from 'chai'
@@ -505,6 +505,8 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
         const TradeFactory: ContractFactory = await ethers.getContractFactory('GnosisTrade')
         trade = <GnosisTrade>await TradeFactory.deploy()
 
+        await setStorageAt(trade.address, 0, 0)
+
         // Check state
         expect(await trade.status()).to.equal(TradeStatus.NOT_STARTED)
         expect(await trade.canSettle()).to.equal(false)
@@ -948,6 +950,8 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
         const TradeFactory: ContractFactory = await ethers.getContractFactory('DutchTrade')
         trade = <DutchTrade>await TradeFactory.deploy()
 
+        await setStorageAt(trade.address, 0, 0)
+
         // Check state
         expect(await trade.status()).to.equal(TradeStatus.NOT_STARTED)
         expect(await trade.canSettle()).to.equal(false)

test/Main.test.ts

@@ -1668,6 +1668,21 @@ describe(`MainP${IMPLEMENTATION} contract`, () => {
         // Basket is now disabled
         expect(await basketHandler.status()).to.equal(CollateralStatus.DISABLED)
       })
+
+      it('Recognizes Sound Collateral', async () => {
+        expect(await collateral1.status()).to.equal(CollateralStatus.SOUND)
+        await expect(assetRegistry.register(collateral1.address)).not.be.reverted
+
+        await revertCollateral.setStatus(CollateralStatus.DISABLED)
+        expect(await revertCollateral.status()).to.equal(CollateralStatus.DISABLED)
+
+        await expect(
+          assetRegistry.connect(owner).register(revertCollateral.address)
+        ).be.revertedWith('collateral not sound')
+        await expect(
+          assetRegistry.connect(owner).swapRegistered(revertCollateral.address)
+        ).be.revertedWith('collateral not sound')
+      })
     })
   })
 

test/integration/EasyAuction.test.ts

@@ -715,10 +715,14 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function
       const CollFactory = await ethers.getContractFactory('FiatCollateral')
       const MainFactory = await ethers.getContractFactory('MainP0')
       const BrokerFactory = await ethers.getContractFactory('BrokerP0')
+      const GnosisTradeFactory = await ethers.getContractFactory('GnosisTrade')
+      const DutchTradeFactory = await ethers.getContractFactory('DutchTrade')
 
       // Deployments
       const main = await MainFactory.deploy()
       const broker = await BrokerFactory.deploy()
+      const gnosisTradeImpl = await GnosisTradeFactory.deploy()
+      const dutchTradeImpl = await DutchTradeFactory.deploy()
       await main.init(
         {
           rToken: ONE_ADDRESS,
@@ -743,9 +747,9 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function
       await broker.init(
         main.address,
         easyAuction.address,
-        ONE_ADDRESS,
+        gnosisTradeImpl.address,
         config.batchAuctionLength,
-        ONE_ADDRESS,
+        dutchTradeImpl.address,
         config.dutchAuctionLength
       )
       const sellTok = await ERC20Factory.deploy('Sell Token', 'SELL', sellTokDecimals)