Escher contest - lumoswiz's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L124 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDAFactory.sol#L33-L36

Vulnerability details

Impact

It is possible to configure the LPDA.Sale parameters in LPDAFactory.sol such that all current checks pass but can lead to underflow of the getPrice() return variable in LPDA.sol part way through the sale duration. Some implications:

• Users can no longer call the buy function, so that the transfers to the saleReceiver and feeReceiver cannot occur.
• Users cannot call refund.

This leads to loss of funds.

Proof of Concept

• Creator sets an endTime 10 days from now and a dropPerSecond of 0.2 ether / 1 days, so that with a startPrice = 1 ether the getPrice function will underflow after 5 days.
• At 5 days, if the sale is still going (temp.currentId != temp.finalId), then getPrice will be calculated from temp.startPrice - (temp.dropPerSecond * timeElapsed).

diff --git a/LPDA.t.sol.orig b/LPDA.t.sol
index c360700..939b47d 100644
--- a/LPDA.t.sol.orig
+++ b/LPDA.t.sol
@@ -188,4 +188,34 @@ contract LPDATest is LPDABase {
         sale.refund();
         assertApproxEqRel(address(this).balance, bal + 0.1 ether, lpdaSale.dropPerSecond);
     }
-}
\ No newline at end of file
+
+    function test_underflowGetPrice() public {
+        // set up a LPDA Sale
+        lpdaSale = LPDA.Sale({
+            currentId: uint48(0),
+            finalId: uint48(10),
+            edition: address(edition),
+            startPrice: uint80(uint256(1 ether)),
+            finalPrice: uint80(uint256(0.1 ether)),
+            dropPerSecond: uint80(uint256(0.2 ether) / 1 days),
+            startTime: uint96(block.timestamp),
+            saleReceiver: payable(address(69)),
+            endTime: uint96(block.timestamp + 10 days)
+        });
+
+        sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
+
+        edition.grantRole(edition.MINTER_ROLE(), address(sale));
+
+        sale.buy{value: 1 ether}(1);
+        sale.buy{value: 4 ether}(3);
+
+        vm.warp(block.timestamp + 6 days);
+
+        vm.expectRevert();
+        sale.buy{value: 1 ether}(1);
+
+        vm.expectRevert();
+        sale.refund();
+    }
+}

Tools Used

Foundry

Recommended Mitigation Steps

• In the getPrice function of LPDA, calculate timeElapsed before the final check on L121. Then change this final check to: if (temp.currentId == temp.finalId || (temp.dropPerSecond * timeElapsed) >= temp.startPrice) return temp.finalPrice;.
• Change the check in LPDAFactory.createLPDASale on L36 to: require(sale.dropPerSecond > 0 && sale.dropPerSecond <= (sale.startPrice - sale.finalPrice) / (sale.endTime - sale.startTime), "INVALID DROP PER SECOND" );.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L86 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L105 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L109 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L92

Vulnerability details

Impact

Use of transfer() in LPDA.sol, FixedPrice.sol and OpenEdition.sol. ETH may be lost if the saleReceiver, msg.sender or feeReceiver are smart contracts that:

• Fail to implement a payable fallback function
• Implements a payable fallback function that uses more than 2300 gas units.

Proof of Concept

References that recommend avoiding the use of transfer() can be found here and here.

Recommended Mitigation Steps

• Use of .call{value: msg.value}("") with checks-effects-interaction pattern and/or using a re-entrancy guard modifier.
  • After the call, have a require statement to check that the ETH was sent.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L81-L87 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L73

Vulnerability details

Impact

The sale ends when the buy function is called and the newly minted tokenId equals the Sale.finalId parameter set in the factory contract. If there isn't sufficient demand for a given target finalId, then the only way for the saleReceiver or feeReceiver to retrieve funds will be to buy up the remaining tokens.

Potential loss of funds if finalId and price is sufficiently high.

Proof of Concept

• If an optimistic creator sets up a sale with currentId = 0, finalId = 1000 and price = 1 ether, and only manage to sell 50 tokens, then there will be no way to recover the 50 ether. To unlock the funds, the remaining 950 units will need to be bought up, costing 950 ether.
• The same problem exists for LPDA sales, but with less overall cost due to decreasing sale price with time.

Recommended Mitigation Steps

• Create an additional function that allows the owner to end the sale when the block.timestamp exceeds the endTime, enabling recovery of funds. Note: FixedPrice.sol will require some way to determine the endTime.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/Escher721.sol#L44 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L74 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L66

Vulnerability details

Impact

The creator is granted MINTER_ROLE in the Escher721 contract initialization. If the creator sets up a sale and, at some point throughout the sale, mints a tokenId through the Escher721 contract such that: currentId + 1 < tokenId <= finalId, this prevents users from being able to call the buy function.

Results in loss of funds because:

• newId == temp.finalId (for LPDA) and newId == sale_.finalId (for fixed price) never eventuate.
• No other way to end the sale and withdraw funds other than the above conditions in the buy functions.

Proof of Concept

diff --git a/LPDA.t.sol.orig b/LPDA.t.sol
index c360700..8720f5c 100644
--- a/LPDA.t.sol.orig
+++ b/LPDA.t.sol
@@ -188,4 +188,33 @@ contract LPDATest is LPDABase {
         sale.refund();
         assertApproxEqRel(address(this).balance, bal + 0.1 ether, lpdaSale.dropPerSecond);
     }
-}
\ No newline at end of file
+
+    function test_creatorMintsTokenIdWithinSaleRangeDuringSale() public {
+        sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
+
+        // sale granted MINTER_ROLE
+        edition.grantRole(edition.MINTER_ROLE(), address(sale));
+
+        // creator mints tokenId = 9 to this address (here the creator is also address(this)).
+        edition.mint(address(this), 9);
+
+        // user buys up to (and including) tokenId = 8
+        sale.buy{value: 8 ether}(8);
+
+        uint256 saleETHBalance = address(sale).balance;
+
+        // skip forward 1 day
+        vm.warp(block.timestamp + 0.5 days);
+
+        // tokenId = 9 has already been minted
+        vm.expectRevert();
+        sale.buy{value: 1 ether}(1);
+
+        // user can still refund
+        sale.refund();
+
+        uint256 unrecoverableETHOwedToCreator = saleETHBalance - address(sale).balance;
+
+        emit log_named_uint("unrecoverable ETH", unrecoverableETHOwedToCreator);
+    }
+}

Tools Used

Foundry

Recommended Mitigation Steps

• Do not grant the creator the MINTER_ROLE in the Escher721 contract either:
  • At any time. Remove _grantRole(MINTER_ROLE, creator) from the initialize function; or
  • During the sale. Call revokeRole(MINTER_ROLE, creator) before the sale commences.