LooksRare Aggregator contest - aviggiano's results

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L32-L38 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L43-L49 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L54-L60

Vulnerability details

Impact

LowLevelETH does not check for transfer status success on _returnETHIfAny(), _returnETHIfAny(address) and _returnETHIfAnyWithOneWeiLeft() functions, which may lead to user loss of funds.

Proof of Concept

A simple example is the following:

1. Alice deploys a contract that interacts with LooksRareAggregator. For some reason, her receive payable external function reverts when receiving ether.
2. Alice executes an order on LooksRareAggregator, and the order fails.
3. Alice's ETH is not returned to her contract, but instead is trapped in LooksRareAggregator
4. Bob calls the execute function in a different order, which is fulfilled. Bob gets Alice's ETH, as a result of _returnETHIfAny

Foundry test showing the issue:

diff --git a/test/foundry/LowLevelETH.t.sol b/test/foundry/LowLevelETH.t.sol
index d925568..8bbb8a7 100644
--- a/test/foundry/LowLevelETH.t.sol
+++ b/test/foundry/LowLevelETH.t.sol
@@ -22,6 +22,15 @@ contract ImplementedLowLevelETH is LowLevelETH {
     }
 }
 
+contract RejectingETH {
+    function transfer (ImplementedLowLevelETH lowLevelETH) external payable {
+        lowLevelETH.transferETHAndReturnFunds{value: msg.value}();
+    }
+    receive() external payable {
+        revert();
+    }
+}
+
 abstract contract TestParameters {
     address internal _sender = address(100);
     address internal _recipient = address(101);
@@ -48,6 +57,15 @@ contract LowLevelETHTest is TestParameters, TestHelpers {
         assertEq(_sender.balance, amount);
     }
 
+    // the following test will not pass due to LowLevelETH contract not checking for `success` on ETH transfer
+    function testTransferETHAndReturnFundsRejectingETHContract(uint112 amount) external payable asPrankedUser(_sender) {
+        vm.assume(amount > 0);
+        vm.deal(_sender, amount);
+        RejectingETH rejecting = new RejectingETH();
+        rejecting.transfer{value:amount}(lowLevelETH);
+        assertEq(address(rejecting).balance, amount);
+        assertEq(address(lowLevelETH).balance, 0);
+    }
+
     function testTransferETHAndReturnFundsToSpecificAddress(uint112 amount) external payable asPrankedUser(_sender) {
         vm.deal(_sender, amount);
         assertEq(_recipient.balance, 0);

Tools Used

Foundry

Recommended Mitigation Steps

Check for success on low-level ETH transfers.

diff --git a/contracts/lowLevelCallers/LowLevelETH.sol b/contracts/lowLevelCallers/LowLevelETH.sol
index d167e10..06e838a 100644
--- a/contracts/lowLevelCallers/LowLevelETH.sol
+++ b/contracts/lowLevelCallers/LowLevelETH.sol
@@ -30,32 +30,44 @@ contract LowLevelETH {
      * @notice Return ETH back to the original sender if any ETH is left in the payable call.
      */
     function _returnETHIfAny() internal {
+        bool status = true;
+
         assembly {
             if gt(selfbalance(), 0) {
-                let status := call(gas(), caller(), selfbalance(), 0, 0, 0, 0)
+                status := call(gas(), caller(), selfbalance(), 0, 0, 0, 0)
             }
         }
+
+        if (!status) revert ETHTransferFail();
     }
 
     /**
      * @notice Return ETH back to the designated sender if any ETH is left in the payable call.
      */
     function _returnETHIfAny(address recipient) internal {
+        bool status = true;
+
         assembly {
             if gt(selfbalance(), 0) {
-                let status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0)
+                status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0)
             }
         }
+
+        if (!status) revert ETHTransferFail();
     }
 
     /**
      * @notice Return ETH to the original sender if any is left in the payable call but leave 1 wei of ETH in the contract.
      */
     function _returnETHIfAnyWithOneWeiLeft() internal {
+        bool status = true;
+
         assembly {
             if gt(selfbalance(), 1) {
-                let status := call(gas(), caller(), sub(selfbalance(), 1), 0, 0, 0, 0)
+                status := call(gas(), caller(), sub(selfbalance(), 1), 0, 0, 0, 0)
             }
         }
+
+        if (!status) revert ETHTransferFail();
     }
 }
diff --git a/test/foundry/LowLevelETH.t.sol b/test/foundry/LowLevelETH.t.sol
index d925568..0613eff 100644
--- a/test/foundry/LowLevelETH.t.sol
+++ b/test/foundry/LowLevelETH.t.sol
@@ -22,6 +22,15 @@ contract ImplementedLowLevelETH is LowLevelETH {
     }
 }
 
+contract RejectingETH {
+    function transfer (ImplementedLowLevelETH lowLevelETH) external payable {
+        lowLevelETH.transferETHAndReturnFunds{value: msg.value}();
+    }
+    receive() external payable {
+        revert();
+    }
+}
+
 abstract contract TestParameters {
     address internal _sender = address(100);
     address internal _recipient = address(101);
@@ -48,6 +57,14 @@ contract LowLevelETHTest is TestParameters, TestHelpers {
         assertEq(_sender.balance, amount);
     }
 
+    function testTransferETHAndReturnFundsRejectingETHContract(uint112 amount) external payable asPrankedUser(_sender) {
+        vm.assume(amount > 0);
+        vm.deal(_sender, amount);
+        RejectingETH rejecting = new RejectingETH();
+        vm.expectRevert(LowLevelETH.ETHTransferFail.selector);
+        rejecting.transfer{value:amount}(lowLevelETH);
+    }
+
     function testTransferETHAndReturnFundsToSpecificAddress(uint112 amount) external payable asPrankedUser(_sender) {
         vm.deal(_sender, amount);
         assertEq(_recipient.balance, 0);

Note: this change will make some mainnet fork tests not pass.

Failing tests:
Encountered 5 failing tests in test/foundry/SeaportProxyBenchmark.t.sol:SeaportProxyBenchmarkTest
[FAIL. Reason: ETHTransferFail()] testExecuteThroughAggregatorSingleOrder() (gas: 5544517)
[FAIL. Reason: ETHTransferFail()] testExecuteThroughAggregatorTwoOrdersAtomic() (gas: 5743719)
[FAIL. Reason: ETHTransferFail()] testExecuteThroughAggregatorTwoOrdersNonAtomic() (gas: 5722068)
[FAIL. Reason: ETHTransferFail()] testExecuteThroughV0AggregatorSingleOrder() (gas: 4429682)
[FAIL. Reason: ETHTransferFail()] testExecuteThroughV0AggregatorTwoOrders() (gas: 4624992)

After inspecting each one of these tests individually, it's possible to see that they are indeed keeping user funds on the aggregator by mistake, due to the sender reverting when receiving ETH.

In order to verify this claim, we can remove all suggested mitigation steps and only add an additional check for aggregator.balance to be equal 0 after execute calls. By doing that, the same tests will revert, proving that their balance is non-null after execute on the current LooksRareAggregator implementation.

diff --git a/test/foundry/LooksRareProxyBenchmark.t.sol b/test/foundry/LooksRareProxyBenchmark.t.sol
index c16d011..54e59f9 100644
--- a/test/foundry/LooksRareProxyBenchmark.t.sol
+++ b/test/foundry/LooksRareProxyBenchmark.t.sol
@@ -121,6 +121,7 @@ contract LooksRareProxyBenchmarkTest is TestParameters, TestHelpers, LooksRarePr
 
         uint256 gasRemaining = gasleft();
         v0Aggregator.execute{value: orders[0].price}(tradeData);
+        assertEq(address(v0Aggregator).balance, 0, "Aggregator should not keep any funds");
         uint256 gasConsumed = gasRemaining - gasleft();
         emit log_named_uint("LooksRare single NFT purchase through the V0 aggregator consumed: ", gasConsumed);
 
diff --git a/test/foundry/SeaportProxyBenchmark.t.sol b/test/foundry/SeaportProxyBenchmark.t.sol
index e16e204..758d6d8 100644
--- a/test/foundry/SeaportProxyBenchmark.t.sol
+++ b/test/foundry/SeaportProxyBenchmark.t.sol
@@ -92,6 +92,7 @@ contract SeaportProxyBenchmarkTest is TestParameters, TestHelpers, SeaportProxyT
 
         uint256 gasRemaining = gasleft();
         aggregator.execute{value: order.price}(tokenTransfers, tradeData, _buyer, _buyer, true);
+        assertEq(address(aggregator).balance, 0, "Aggregator should not keep any funds");
         uint256 gasConsumed = gasRemaining - gasleft();
         emit log_named_uint("Seaport single NFT purchase through the aggregator consumed: ", gasConsumed);
 
@@ -128,6 +129,7 @@ contract SeaportProxyBenchmarkTest is TestParameters, TestHelpers, SeaportProxyT
 
         uint256 gasRemaining = gasleft();
         v0Aggregator.execute{value: order.price}(tradeData);
+        assertEq(address(v0Aggregator).balance, 0, "Aggregator should not keep any funds");
         uint256 gasConsumed = gasRemaining - gasleft();
         emit log_named_uint("Seaport single NFT purchase through the V0 aggregator consumed: ", gasConsumed);
 
@@ -269,6 +271,7 @@ contract SeaportProxyBenchmarkTest is TestParameters, TestHelpers, SeaportProxyT
 
         uint256 gasRemaining = gasleft();
         v0Aggregator.execute{value: totalPrice}(tradeData);
+        assertEq(address(v0Aggregator).balance, 0, "Aggregator should not keep any funds");
         uint256 gasConsumed = gasRemaining - gasleft();
         emit log_named_uint("Seaport multiple NFT purchase through the V0 aggregator consumed: ", gasConsumed);
 
@@ -329,6 +332,7 @@ contract SeaportProxyBenchmarkTest is TestParameters, TestHelpers, SeaportProxyT
 
         uint256 gasRemaining = gasleft();
         aggregator.execute{value: totalPrice}(new TokenTransfer[](0), tradeData, _buyer, _buyer, isAtomic);
+        assertEq(address(aggregator).balance, 0, "Aggregator should not keep any funds");
         uint256 gasConsumed = gasRemaining - gasleft();
 
         if (isAtomic) {

The above changes will make these tests revert, proving that the aggregator is keeping some ETH after execute calls.

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L109 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L220

Vulnerability details

Impact

Trapped ETH inside LooksRareAggregator cannot be rescued by owner, as the contract only implements rescueERC721 and rescueERC1155, but not an equivalent rescueETH.

As a result, an attacker can steal any trapped ETH simply by calling execute on any order and receive the total balance of the contract via the _returnETHIfAny function at the end of the execute method from LooksRareAggregator.

Proof of Concept

1. Alice mistakenly sends ETH to LooksRareAggregator
2. The contract owner cannot rescue Alice's ETH
3. Bob creates an order costing only 1 wei and allowlists his other wallet
4. Bob calls execute and successfully fills the order. The contract's balance (consisting of Alice's ETH) will be transferred to Bob

Tools Used

Foundry

Recommended Mitigation Steps

• Implement rescueETH following the other methods rescueERC721 and rescueERC1155.
• Remove the receive function from the contract

1. Cache orderExtraData.recipients[j] on SeaportProxy._populateParameters

Code changes

diff --git a/contracts/proxies/SeaportProxy.sol b/contracts/proxies/SeaportProxy.sol
index ab06a83..246d502 100644
--- a/contracts/proxies/SeaportProxy.sol
+++ b/contracts/proxies/SeaportProxy.sol
@@ -253,11 +253,12 @@ contract SeaportProxy is IProxy, TokenRescuer {
 
         ConsiderationItem[] memory consideration = new ConsiderationItem[](recipientsLength);
         for (uint256 j; j < recipientsLength; ) {
+            AdditionalRecipient memory recipient = orderExtraData.recipients[j];
             // We don't need to assign value to identifierOrCriteria as it is always 0.
-            uint256 recipientAmount = orderExtraData.recipients[j].amount;
+            uint256 recipientAmount = recipient.amount;
             consideration[j].startAmount = recipientAmount;
             consideration[j].endAmount = recipientAmount;
-            consideration[j].recipient = payable(orderExtraData.recipients[j].recipient);
+            consideration[j].recipient = payable(recipient.recipient);
             consideration[j].itemType = order.currency == address(0) ? ItemType.NATIVE : ItemType.ERC20;
             consideration[j].token = order.currency;

Gas changes

$ FOUNDRY_PROFILE=local forge snapshot --diff
// ...
Overall gas change: -61072 (-3.277%)

2. Cache singleTradeData.proxy on LooksRareAggregator.execute

Code changes

diff --git a/contracts/LooksRareAggregator.sol b/contracts/LooksRareAggregator.sol
index f215f39..2ade000 100644
--- a/contracts/LooksRareAggregator.sol
+++ b/contracts/LooksRareAggregator.sol
@@ -68,7 +68,8 @@ contract LooksRareAggregator is
 
         for (uint256 i; i < tradeDataLength; ) {
             TradeData calldata singleTradeData = tradeData[i];
-            if (!_proxyFunctionSelectors[singleTradeData.proxy][singleTradeData.selector]) revert InvalidFunction();
+            address proxy = singleTradeData.proxy;
+            if (!_proxyFunctionSelectors[proxy][singleTradeData.selector]) revert InvalidFunction();
 
             (bytes memory proxyCalldata, bool maxFeeBpViolated) = _encodeCalldataAndValidateFeeBp(
                 singleTradeData,
@@ -85,7 +86,7 @@ contract LooksRareAggregator is
                     continue;
                 }
             }
-            (bool success, bytes memory returnData) = singleTradeData.proxy.delegatecall(proxyCalldata);
+            (bool success, bytes memory returnData) = proxy.delegatecall(proxyCalldata);
 
             if (!success) {
                 if (isAtomic) {

Gas changes

$ FOUNDRY_PROFILE=local forge snapshot --diff
// ...
Overall gas change: -96598 (-5.302%)