Trade does not reverts even if isAtomic flag is set, when calling non-existent proxy.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L88-L101

Vulnerability details

Impact

Buyers can set the bool flag isAtomic to disable partial executions. But if the proxy address doesn't exist (e.g a previously approved valid proxy was selfdestructed or owner aproved the wrong address by mistake), the delegatecall to this non-existent proxy will return true (delegatecall does not rethrow exceptions to their caller), therefore even if isAtomic is set to true the execute function will not revert and partial executions will go through.

Proof of Concept

The following test expects a revert as we are calling execute with a valid trade, isAtomic flag set, but on a non-existent proxy. But as discussed above, it does not revert and test fail.

diff --git a/LooksRareAggregatorTrades.t.sol.orig b/LooksRareAggregatorTrades.t.sol
index d1dcf91..7216706 100644
--- a/LooksRareAggregatorTrades.t.sol.orig
+++ b/LooksRareAggregatorTrades.t.sol
@@ -108,4 +108,43 @@ contract LooksRareAggregatorTradesTest is

         vm.stopPrank();
     }
+
+    function testAtomicExecuteOnNonExistentContract() public {
+        vm.createSelectFork(vm.rpcUrl("mainnet"), 15_320_038);
+
+        aggregator = new LooksRareAggregator();
+        LooksRareProxy looksRareProxy = new LooksRareProxy(LOOKSRARE_V1, address(aggregator));
+        aggregator.addFunction(address(looksRareProxy), LooksRareProxy.execute.selector);
+        // Since we are forking mainnet, we have to make sure it has 0 ETH.
+        vm.deal(address(looksRareProxy), 0);
+
+        bytes memory empty;
+        // Same as calling selfdestruct.
+        vm.etch(address(looksRareProxy), empty);
+
+        TokenTransfer[] memory tokenTransfers = new TokenTransfer[](0);
+        BasicOrder[] memory validOrders = validBAYCOrders();
+        BasicOrder[] memory orders = new BasicOrder[](1);
+        orders[0] = validOrders[0];
+
+        bytes[] memory ordersExtraData = new bytes[](1);
+        ordersExtraData[0] = abi.encode(orders[0].price, 9550, 0, LOOKSRARE_STRATEGY_FIXED_PRICE);
+
+        ILooksRareAggregator.TradeData[] memory tradeData = new ILooksRareAggregator.TradeData[](1);
+        tradeData[0] = ILooksRareAggregator.TradeData({
+            proxy: address(looksRareProxy),
+            selector: LooksRareProxy.execute.selector,
+            value: orders[0].price,
+            maxFeeBp: 0,
+            orders: orders,
+            ordersExtraData: ordersExtraData,
+            extraData: ""
+        });
+
+        vm.deal(_buyer, orders[0].price);
+        vm.prank(_buyer);
+
+        vm.expectRevert();
+        aggregator.execute{value: orders[0].price}(tokenTransfers, tradeData, address(0), _buyer, true);
+    }
 }

Recommended Mitigation Steps

Please consider adding a check for proxy existence before calling delegatecall.

[Picodes]

In case the proxy is self destructed or an error is made by the owner, the call should do nothing. Also there is already a check here assuming the owner behaves correctly

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-sponsor]

0xhiroshi marked the issue as sponsor disputed

[c4-judge]

Picodes marked the issue as grade-c