4.2.0 - Fixes from audit

common/constants.ts

@@ -64,6 +64,7 @@ export enum BidType {
   NONE,
   CALLBACK,
   TRANSFER,
+  FILL,
 }
 
 export const FURNACE_DEST = '0x0000000000000000000000000000000000000001'

contracts/p1/mixins/GlobalReentrancyGuard.sol

@@ -77,13 +77,4 @@ abstract contract GlobalReentrancyGuard is Initializable {
         // https://eips.ethereum.org/EIPS/eip-2200)
         $._status = NOT_ENTERED;
     }
-
-    /**
-     * @dev Returns true if the reentrancy guard is currently set to "entered", which indicates
-     *  there is a `nonReentrant` function in the call stack.
-     */
-    function _reentrancyGuardEntered() internal view returns (bool) {
-        ReentrancyGuardStorage storage $ = _getReentrancyGuardStorage();
-        return $._status == ENTERED;
-    }
 }

contracts/plugins/mocks/CowSwapFillerMock.sol

@@ -45,6 +45,9 @@ contract CowSwapFillerMock is Initializable, IBaseTrustedFiller {
     uint256 public price; // D27{buyTok/sellTok}
     bool public partiallyFillable;
 
+    // mock: allow to force swapActive on tests
+    bool public forceSwapActive;
+
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor() {
         _disableInitializers();
@@ -93,6 +96,11 @@ contract CowSwapFillerMock is Initializable, IBaseTrustedFiller {
 
     /// @return true if the contract is mid-swap and funds have not yet settled
     function swapActive() public view returns (bool) {
+        // mock: used for testing
+        if (forceSwapActive) {
+            return true;
+        }
+
         if (block.number != blockInitialized) {
             return false;
         }
@@ -135,4 +143,9 @@ contract CowSwapFillerMock is Initializable, IBaseTrustedFiller {
             token.safeTransfer(fillCreator, tokenBalance);
         }
     }
+
+    /// Mock: Setter for forceSwapActive
+    function setForceSwapActive(bool _forceSwapActive) external {
+        forceSwapActive = _forceSwapActive;
+    }
 }

contracts/plugins/trading/DutchTrade.sol

@@ -23,7 +23,8 @@ interface IDutchTradeCallee {
 enum BidType {
     NONE,
     CALLBACK,
-    TRANSFER
+    TRANSFER,
+    FILL
 }
 
 // A dutch auction in 4 parts:
@@ -123,6 +124,7 @@ contract DutchTrade is ITrade, Versioned {
 
     // === Trusted Fillers ===
     IBaseTrustedFiller public activeTrustedFill;
+    uint192 public savedFillPrice; // cached trusted fill price
 
     // This modifier both enforces the state-machine pattern and guards against reentrancy.
     modifier stateTransition(TradeStatus begin, TradeStatus end) {
@@ -303,6 +305,7 @@ contract DutchTrade is ITrade, Versioned {
         closeTrustedFiller
         returns (IBaseTrustedFiller filler)
     {
+        require(bidder == address(0), "bid already received");
         require(status == TradeStatus.OPEN, "trade not open");
 
         // Get trusted filler registry
@@ -311,9 +314,12 @@ contract DutchTrade is ITrade, Versioned {
         require(address(registry) != address(0) && enabled, "trusted fillers not enabled");
 
         // Get current price and amounts
-        uint192 price = _price(uint48(block.timestamp));
+        savedFillPrice = _price(uint48(block.timestamp));
         uint256 sellAmt = lot(); // {qSellTok}
-        uint256 buyAmt = _bidAmount(price); // {qBuyTok}
+        uint256 buyAmt = _bidAmount(savedFillPrice); // {qBuyTok}
+
+        // Mark bid type
+        bidType = BidType.FILL;
 
         // Create trusted filler
         filler = registry.createTrustedFiller(msg.sender, targetFiller, deploymentSalt);
@@ -341,23 +347,26 @@ contract DutchTrade is ITrade, Versioned {
     {
         require(msg.sender == address(origin), "only origin can settle");
 
-        // If auction has ended -> continue
-        if (block.timestamp <= endTime) {
-            bool filled = false;
-            if (block.timestamp >= startTime) {
-                // Ongoing auction, check if can be settled
-                uint192 price = _price(uint48(block.timestamp));
-                uint256 amountIn = _bidAmount(price);
-                filled = buy.balanceOf(address(this)) >= amountIn;
+        bool filled = false;
+        if (bidType == BidType.FILL) {
+            uint256 amountIn = _bidAmount(savedFillPrice);
+            filled = buy.balanceOf(address(this)) >= amountIn;
+
+            // reportViolation if filled in geometric phase
+            if (filled && savedFillPrice > bestPrice.mul(ONE_POINT_FIVE, CEIL)) {
+                broker.reportViolation();
             }
-            require(bidder != address(0) || filled, "auction not over");
         }
 
+        require(bidder != address(0) || filled || block.timestamp > endTime, "auction not over");
+
         if (bidType == BidType.CALLBACK) {
             soldAmt = lot(); // {qSellTok}
         } else if (bidType == BidType.TRANSFER) {
             soldAmt = lot(); // {qSellTok}
             sell.safeTransfer(bidder, soldAmt); // {qSellTok}
+        } else if (bidType == BidType.FILL && filled) {
+            soldAmt = lot(); // {qSellTok}
         }
 
         // Transfer remaining balances back to origin
@@ -385,17 +394,23 @@ contract DutchTrade is ITrade, Versioned {
         if (block.timestamp > endTime) {
             return true;
         }
+        // OPEN with active fill -> false
+        if (address(activeTrustedFill) != address(0) && activeTrustedFill.swapActive()) {
+            return false;
+        }
 
-        // Ongoing OPEN auction, check if can be settled
-        uint192 price = _price(uint48(block.timestamp));
-        uint256 amountIn = _bidAmount(price);
-
-        uint256 amountInFiller = address(activeTrustedFill) != address(0)
-            ? buy.balanceOf(address(activeTrustedFill))
-            : 0;
-        uint256 amountInTrade = buy.balanceOf(address(this));
+        // Ongoing OPEN auction, no active fill, check if can be settled
+        bool filled = false;
+        if (bidType == BidType.FILL) {
+            uint256 amountIn = _bidAmount(savedFillPrice);
+            uint256 amountInFiller = address(activeTrustedFill) != address(0)
+                ? buy.balanceOf(address(activeTrustedFill))
+                : 0;
+            uint256 amountInTrade = buy.balanceOf(address(this));
+            filled = amountInFiller + amountInTrade >= amountIn;
+        }
 
-        return (bidder != address(0) || amountInFiller + amountInTrade >= amountIn);
+        return (bidder != address(0) || filled);
     }
 
     // === Private ===

package.json

@@ -118,7 +118,7 @@
     "@aave/core-v3": "^1.18.0",
     "@aave/periphery-v3": "^2.5.0",
     "@nomicfoundation/hardhat-toolbox": "^2.0.1",
-    "@reserve-protocol/trusted-fillers": "github:reserve-protocol/trusted-fillers#6ac118cc4a353e17712964d194814efce878363d",
+    "@reserve-protocol/trusted-fillers": "github:reserve-protocol/trusted-fillers#a3fdf80204aa2915be313641590ff5c3be9a6c8e",
     "@types/isomorphic-fetch": "^0.0.36",
     "axios-retry": "^4.1.0",
     "cheerio": "^1.0.0-rc.12",

test/Broker.test.ts

@@ -1574,9 +1574,12 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
 
       const getNextTradeAddress = async (tradeRequest: ITradeRequest): Promise<string> => {
         let tradeAddress = ''
+        const sellColl = await ethers.getContractAt('ICollateral', tradeRequest.sell)
+        const sellToken = await ethers.getContractAt('ERC20Mock', await sellColl.erc20())
+
         await whileImpersonating(backingManager.address, async (bmSigner) => {
           // set approval for broker
-          await token0.connect(bmSigner).approve(broker.address, tradeRequest.sellAmount)
+          await sellToken.connect(bmSigner).approve(broker.address, tradeRequest.sellAmount)
           tradeAddress = await broker.connect(bmSigner).callStatic.openTrade(
             TradeKind.BATCH_AUCTION, // workaround to by-pass price checks
             tradeRequest,
@@ -1626,6 +1629,9 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
         expect(await dutchTrade.status()).to.equal(TradeStatus.OPEN)
         expect(await dutchTrade.activeTrustedFill()).to.equal(ZERO_ADDRESS)
 
+        expect(await dutchTrade.bidType()).to.equal(BidType.NONE)
+        expect(await dutchTrade.savedFillPrice()).to.equal(0)
+
         // Create trusted fill
         await expect(
           dutchTrade
@@ -1637,6 +1643,11 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
         const activeFill = await dutchTrade.activeTrustedFill()
         expect(activeFill).to.not.equal(ZERO_ADDRESS)
 
+        // Verify price was cached and type set
+        expect(await dutchTrade.bidType()).to.equal(BidType.FILL)
+        const prevSavedFillPrice = await dutchTrade.savedFillPrice()
+        expect(prevSavedFillPrice).to.be.gt(0)
+
         // Try to create second fill - will close previous one
         const prevFill = activeFill
         await expect(
@@ -1648,6 +1659,9 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
         // Check new trusted fill status
         expect(await dutchTrade.activeTrustedFill()).to.not.equal(ZERO_ADDRESS)
         expect(await dutchTrade.activeTrustedFill()).to.not.equal(prevFill)
+
+        // New price was cached
+        expect(await dutchTrade.savedFillPrice()).to.be.lt(prevSavedFillPrice)
       })
 
       it('Should not create trusted fill when registry not enabled', async () => {
@@ -1721,6 +1735,9 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
         expect(await dutchTrade.status()).to.equal(TradeStatus.OPEN)
         expect(await dutchTrade.activeTrustedFill()).to.equal(ZERO_ADDRESS)
 
+        expect(await dutchTrade.bidType()).to.equal(BidType.NONE)
+        expect(await dutchTrade.savedFillPrice()).to.equal(0)
+
         // Check balances
         expect(await token0.balanceOf(backingManager.address)).to.equal(
           initBal0.sub(tradeRequest.sellAmount)
@@ -1732,6 +1749,14 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
           .connect(addr1)
           .createTrustedFill(cowSwapFillerMock.address, ethers.utils.randomBytes(32))
 
+        // Use cached price at creation
+        const bidAmount = await dutchTrade.bidAmount(await getLatestBlockTimestamp())
+
+        // Verify price was cached and type set
+        expect(await dutchTrade.bidType()).to.equal(BidType.FILL)
+        const prevSavedFillPrice = await dutchTrade.savedFillPrice()
+        expect(prevSavedFillPrice).to.be.gt(0)
+
         const activeFill = await dutchTrade.activeTrustedFill()
         expect(activeFill).to.not.equal(ZERO_ADDRESS)
 
@@ -1740,7 +1765,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
 
         // Perform fill
         await token0.burn(activeFill, tradeRequest.sellAmount)
-        const bidAmount = await dutchTrade.bidAmount(await getLatestBlockTimestamp())
         await token1.mint(activeFill, bidAmount)
 
         // The trade should be settleable with an active trusted fill
@@ -1751,6 +1775,9 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
           await expect(dutchTrade.connect(bmSigner).settle()).to.not.be.reverted
         })
 
+        // Used cached price
+        expect(await dutchTrade.savedFillPrice()).to.equal(prevSavedFillPrice)
+
         // After settling, the trusted fill should be closed
         expect(await dutchTrade.activeTrustedFill()).to.equal(ZERO_ADDRESS)
         expect(await dutchTrade.status()).to.equal(TradeStatus.CLOSED)
@@ -1816,6 +1843,7 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
         expect(await token1.balanceOf(activeFill)).to.be.lt(tradeRequest.minBuyAmount)
 
         // The trade should not be settleable
+        expect(await dutchTrade.bidType()).to.equal(BidType.FILL)
         expect(await dutchTrade.canSettle()).to.equal(false)
 
         // Cannot settle the trade
@@ -1843,6 +1871,81 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => {
           initBal1.add(tradeRequest.minBuyAmount.div(2))
         )
       })
+
+      it('Should not allow to settle trade if swap active', async () => {
+        // Create and setup a Dutch trade
+        const tradeAddress = await getNextTradeAddress(tradeRequest)
+
+        // Get current balances
+        const initBal0 = await token0.balanceOf(backingManager.address)
+        const initBal1 = await token1.balanceOf(backingManager.address)
+
+        // Set automine to false
+        await hre.network.provider.send('evm_setAutomine', [false])
+
+        // Create Dutch trade (need to refresh collaterals in the same timestamp)
+        await assetRegistry.refresh()
+        await whileImpersonating(backingManager.address, async (bmSigner) => {
+          await token0.connect(bmSigner).approve(broker.address, tradeRequest.sellAmount)
+          await broker.connect(bmSigner).openTrade(TradeKind.DUTCH_AUCTION, tradeRequest, prices)
+        })
+
+        // Mine block and reset automine
+        await hre.network.provider.send('evm_mine', [])
+        await hre.network.provider.send('evm_setAutomine', [true])
+
+        // Check trade status
+        dutchTrade = await ethers.getContractAt('DutchTrade', tradeAddress)
+        expect(await dutchTrade.status()).to.equal(TradeStatus.OPEN)
+        expect(await dutchTrade.activeTrustedFill()).to.equal(ZERO_ADDRESS)
+
+        // Check balances
+        expect(await token0.balanceOf(backingManager.address)).to.equal(
+          initBal0.sub(tradeRequest.sellAmount)
+        )
+        expect(await token1.balanceOf(backingManager.address)).to.equal(initBal1)
+
+        // Create trusted fill
+        await dutchTrade
+          .connect(addr1)
+          .createTrustedFill(cowSwapFillerMock.address, ethers.utils.randomBytes(32))
+
+        // Use cached price at creation
+        const bidAmount = await dutchTrade.bidAmount(await getLatestBlockTimestamp())
+
+        // Set swap active
+        const activeFill = await dutchTrade.activeTrustedFill()
+        const cowswapFillerMock = await ethers.getContractAt('CowSwapFillerMock', activeFill)
+        await cowswapFillerMock.setForceSwapActive(true)
+
+        // Perform fill
+        await token0.burn(activeFill, tradeRequest.sellAmount)
+        await token1.mint(activeFill, bidAmount)
+
+        // Cannot settle at this point
+        await expect(await dutchTrade.canSettle()).to.equal(false)
+
+        //  End swap active
+        await cowswapFillerMock.setForceSwapActive(false)
+
+        // The trade should now be settleable
+        expect(await dutchTrade.canSettle()).to.equal(true)
+
+        // Settle the trade
+        await whileImpersonating(backingManager.address, async (bmSigner) => {
+          await expect(dutchTrade.connect(bmSigner).settle()).to.not.be.reverted
+        })
+
+        // After settling, the trusted fill should be closed
+        expect(await dutchTrade.activeTrustedFill()).to.equal(ZERO_ADDRESS)
+        expect(await dutchTrade.status()).to.equal(TradeStatus.CLOSED)
+
+        // Check balances
+        expect(await token0.balanceOf(backingManager.address)).to.equal(
+          initBal0.sub(tradeRequest.sellAmount)
+        )
+        expect(await token1.balanceOf(backingManager.address)).to.equal(initBal1.add(bidAmount))
+      })
     })
   })