Escher contest - danyams's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L63 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L101 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L124

Vulnerability details

Impact

All ether sent to an LPDA contract can become stuck due to an arithmetic underflow error. This error stems from getPrice( ). Specifically, this function will throw an error if sale.startPrice < ((sale.endTime - sale.startTime) * sale.dropPerSecond) and a sufficcieint amount of time has passed.

function getPrice() public view returns (uint256) {
    Sale memory temp = sale;
    (uint256 start, uint256 end) = (temp.startTime, temp.endTime);
    ...

    uint256 timeElapsed = end > block.timestamp ? block.timestamp - start : end - start;
    return temp.startPrice - (temp.dropPerSecond * timeElapsed);
}

Since both refund( ) and buy( ) both call getPrice( ), both functions will throw errors and will not execute.

 function buy(uint256 _amount) external payable {
    ...
    if (block.timestamp < temp.startTime) revert TooSoon();
    uint256 price = getPrice();
   ...
}

function refund() public {
    Receipt memory r = receipts[msg.sender];
    uint80 price = uint80(getPrice()) * r.amount;
    ...
}

Proof of Concept

1. Call createLPDASale( ) from LPDAFactoory with a sale, such that:

sale.startPrice < ((sale.endTime - sale.startTime) * sale.dropPerSecond)

2. Wait till sale.endTime + 1
3. Call refund( ) or buy( ) from the LPDA contract

Here is the proof of concept from the given test suite:

--- a/test/LPDA.t.sol
+++ b/test/LPDA.t.sol
@@ -22,7 +22,7 @@ contract LPDABase is EscherTest {
         dropPerSecond: uint80(uint256(0.1 ether) / 1 days),
         startTime: uint96(block.timestamp),
         saleReceiver: payable(address(69)),
-            endTime: uint96(block.timestamp + 1 days)
+            endTime: uint96(block.timestamp + 10 days + 1)
     });
 }
 }
@@ -70,9 +70,10 @@ contract LPDATest is LPDABase {
     sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
     // authorize the lpda sale to mint tokens
     edition.grantRole(edition.MINTER_ROLE(), address(sale));
+        vm.warp(lpdaSale.endTime);
+        vm.expectRevert(stdError.arithmeticError);
     //lets buy an NFT
     sale.buy{value: 1 ether}(1);
-        assertEq(address(sale).balance, 1 ether);
 }

 function test_RevertsWhenTooSoon_Buy() public {
@@ -136,9 +137,15 @@ contract LPDATest is LPDABase {
 }

 function test_Refund() public {
-        test_Buy();
+        sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
+        // authorize the lpda sale to mint tokens
+        edition.grantRole(edition.MINTER_ROLE(), address(sale));
+        //lets buy an NFT
+        sale.buy{value: 1 ether}(1);
+        assertEq(address(sale).balance, 1 ether);

-        vm.warp(lpdaSale.endTime + 1);
+        vm.warp(lpdaSale.endTime);
+        vm.expectRevert(stdError.arithmeticError);
          sale.refund();
  }

Tools Used

Foundry

Recommended Mitigation Steps

In place of attempting to do subtraction, getPrice( ) should return 0 if temp.dropPerSecond * timeElapsed > temp.startPrice.

--- a/src/minters/LPDA.sol
+++ b/src/minters/LPDA.sol
@@ -121,25 +131,26 
     if (temp.currentId == temp.finalId) return temp.finalPrice;

     uint256 timeElapsed = end > block.timestamp ? block.timestamp - start : end - start;
-         return temp.startPrice - (temp.dropPerSecond * timeElapsed);
+        return temp.dropPerSecond * timeElapsed > temp.startPrice ? 0 : temp.startPrice - (temp.dropPerSecond * timeElapsed);
 }

Alternatively, a require statement can be added in createLPDASale, so that these types of sales are not possible.

--- a/src/minters/LPDAFactory.sol
+++ b/src/minters/LPDAFactory.sol
@@ -34,6 +34,7 @@ contract LPDAFactory is Ownable {
     require(sale.finalId > sale.currentId, "INVALID FINAL ID");
     require(sale.startPrice > 0, "INVALID START PRICE");
     require(sale.dropPerSecond > 0, "INVALID DROP PER SECOND");
+    require(sale.startPrice >= (sale.endTime - sale.startTime) * sale.dropPerSecond, "INVALID PARAMS");

     clone = implementation.clone();
     LPDA(clone).initialize(sale);

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPriceFactory.sol#L30 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEditionFactory.sol#L30 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDAFactory.sol#L30

Vulnerability details

Impact

Anyone can create an open edition, fixed price, or LDSA sale without being a creator. The msg.sender only needs to provide a create a contract that returns true when hasRole(bytes32, address) is queried.

Considering that no contract keeps track of active sales, sales are likely being tracked through events. This means anyone could feign an Escher721 sale on Escher's frontend depending on the implementation.

Proof of Concept

1. Create a contract that contains an external function, hasRole(bytes32, address), that always returns true

2. Call createFixedSale( ), createLPDASale( ), or createOpenEdition( ) from FixedPriceFactory, LPDAFactory, or OpenEditionFactory, respectively, with a valid sale and the aforementioned contract as the sale's edition

   import "forge-std/Test.sol";
   import {LPDAFactory, LPDA} from "src/minters/LPDAFactory.sol";
   import {OpenEditionFactory, OpenEdition} from "src/minters/OpenEditionFactory.sol";
   import {FixedPriceFactory, FixedPrice} from "src/minters/FixedPriceFactory.sol";
   
    contract ExploiterContract {
        function hasRole(bytes32, address) external view returns(bool) {
            return true;
        }
    }
   
    contract CreateSaleBugs is Test {
    function testCreateInvalidFixedPriceSale() public {
   
        FixedPriceFactory _factory = new FixedPriceFactory();
        address _exploiter = address(new ExploiterContract());
   
        FixedPrice.Sale memory _validSale = FixedPrice.Sale({
            currentId: uint48(0),
            finalId: uint48(1),
            edition: _exploiter,
            price: uint96(0),
            saleReceiver: payable(address(1)),
            startTime: uint96(block.timestamp)
        });
   
        _factory.createFixedSale(_validSale);
    }
   
    function testCreateInvalidLPDASale() public {
   
    LPDAFactory _factory = new LPDAFactory();
    address _exploiter = address(new ExploiterContract());
   
    LPDA.Sale memory _validSale = LPDA.Sale({
        currentId: uint48(0),
        finalId: uint48(1),
        edition: _exploiter,
        startPrice: uint80(1 ether),
        finalPrice: uint80(0),
        dropPerSecond: 1,
        startTime: uint96(block.timestamp),
        saleReceiver: payable(address(1)),
        endTime: uint96(block.timestamp + 1 days)
    });
   
    _factory.createLPDASale(_validSale);
   }
   
    function testCreateInvalidOpenEditionSale() public {
   
    OpenEditionFactory _factory = new OpenEditionFactory();
    address _exploiter = address(new ExploiterContract());
   
    OpenEdition.Sale memory _validSale = OpenEdition.Sale({
        price: uint72(0),
        currentId: uint24(1),
        edition: _exploiter,
        startTime: uint96(block.timestamp),
        saleReceiver: payable(address(1)),
        endTime: uint96(block.timestamp + 1 days)
    });
   
    _factory.createOpenEdition(_validSale);
    }
    }
   

Recommended Mitigation Steps

Check if msg.sender is a creator from the Escher contract. Relying on the NFT contract, itself, for authorization will enable exploiters to create sales without being a creator.

--- a/src/minters/FixedPriceFactory.sol
+++ b/src/minters/FixedPriceFactory.sol
@@ -5,12 +5,14 @@ import {FixedPrice} from "./FixedPrice.sol";
 import {Ownable} from "openzeppelin/access/Ownable.sol";
 import {Clones} from "openzeppelin/proxy/Clones.sol";
 import {IEscher721} from "../interfaces/IEscher721.sol";
+import {Escher} from "../Escher.sol";

 contract FixedPriceFactory is Ownable {
     using Clones for address;

     address payable public feeReceiver;
     address public immutable implementation;
+    Escher public immutable escher;

     event NewFixedPriceContract(
         address indexed creator,
@@ -19,15 +21,16 @@ contract FixedPriceFactory is Ownable {
        FixedPrice.Sale saleInfo
     );

-    constructor() Ownable() {
+    constructor(address _escher) Ownable() {
           implementation = address(new FixedPrice());
          feeReceiver = payable(msg.sender);
+        escher = Escher(_escher);
      }

 /// @notice create a fixed sale proxy contract
 /// @param _sale the sale info
 function createFixedSale(FixedPrice.Sale calldata _sale) external returns (address clone) {
-        require(IEscher721(_sale.edition).hasRole(bytes32(0x00), msg.sender), "NOT AUTHORIZED");
+        require(escher.hasRole(escher.CREATOR_ROLE(), msg.sender), "NOT AUTHORIZED");
         require(_sale.startTime >= block.timestamp, "START TIME IN PAST");
         require(_sale.finalId > _sale.currentId, "FINAL ID BEFORE CURRENT");

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L50-L53 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L73

Vulnerability details

Impact

Funds in a fixed-price sale will become stuck if demand is overestimated. Under the current implementation, funds are only ever transferred out of the contract when the internal function, _end( ), is called. However, this function is only callable under 2 conditions: in cancel() when the sale start time is less than the current block timestamp, and in buy() when the last id of the purchased NFTs is equal to the sale's final id.

Cancel() is never callable once the sale starts. The primary vulnerability occurs when the sale's final id is never reached from overestimating demand.

Proof of Concept

1. Create a sale such that the fixed price is 1 ether and the final id is 1000
2. Have demand only be for 100 NFTs
3. The 100 ETH is now stuck until the remaining 900 NFTs are bought

Recommended Mitigation Steps

Delete the require statement in cancel( ) that checks if block timestamp is less than the sale's start time.

@@ -48,7 +55,6 @@ contract FixedPrice is Initializable, OwnableUpgradeable, ISale {

     /// @notice Owner can cancel current sale
     function cancel() external onlyOwner {
-        require(block.timestamp < sale.startTime, "TOO LATE");
         _end(sale);
 }

This makes it so that the owner can cancel the sale at any time and transfer the ether to both the fee receiver and sale receiver.

Alternatively, add a withdraw( ) function that transfers the fee to the fee receiver and the remaining balance to the sale receiver.

@@ -109,4 +117,10 @@ contract FixedPrice is Initializable, OwnableUpgradeable, ISale {
         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
         selfdestruct(_sale.saleReceiver);
     }
+
+    
+    function withdraw() external onlyOwner {
+        ISaleFactory(factory).feeReceiver().transfer(address(this).balance/20);
+        sale.saleReceiver.transfer(address(this).balance);
+    }
  }

[01] Buyers of LPDA risk not being refunded

If an LPDA sale has greater demand than expected, a user may never be refunded. This occurs since the balance of the contract will be transferred out in the buy( ) function once the sale's final id is reached.

Recommended Mitigation Steps:

A solution would be to separate the transfers into a separate function and make it callable only after the sale's end time has been reached.

Referenced Code: https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L81-L88

[02] Open Editions cannot be sold for more than 4722 ether

OpenEdition.sol has a struct Sale, that contains 2^72 for its member variable, price. 2^72 wei is roughly 4722 ether, meaning that this is the max limit that an Open Edition NFT can be sold for.

Recommended Mitigation Steps:

Change price to a uint80, which would lead to a max price of 1208925 ether.

Referenced Code: https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L15

[03] Sales and Escher721 all use mint( ) instead of safeMint( )

Using safeMint( ) rather than mint( ) leads to better composability and expected behavior when the caller is another contract.

[04] Transfer function is used to transfer ether

It is better to use call( ) to send Ether since gas costs of opcodes are susceptible to change.

[05] block.timestamp and block.number are used for entropy

block.timestamp and block.number are used for entropy in Generative.sol. This will lead to two Generative contracts that both call setSeedBase() in the same block to have the same seedbase. An oracle is a more reliable source of entropy.

Referenced Code: https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/uris/Generative.sol#L22-L25

[06] uints are not safely casted

All casting of uints should use a safe cast library, such as Solmate's SafeCastLib. Not doing so leads to strange behavior when buy( ) is called in a FixedPrice sale.

A function parameter, _amount, is a uint256. _amount is later casted to a uint48, but this casted value is not used when emitting an event. This means that 2^48 can be passed in as an argument, which will later cast to 0 and no NFTs being purchased. However, an event will be emitted that 2^48 NFTs were bought.

Referenced Code: https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L57-L71

[07] Public functions that are not used internally should use the visibility modifier external

Numerous functions throughout the codebase have a visibility modifier of public when they should use external

[08] Reference types that are used as function arguments should use calldata instead of memory

There are many reference types that are passed in as function arguments do not change in value. They should be marked as calldata in place of memory. Specifically, this occurs frequently when a struct of type Sale is passed in as a function argument

[09] Unnecessary Inheritance

OpenEdition, FixedPrice, LPDA do not need to inherit Initializable since it is already inherited in OwnableUpgradable. Escher721 also does not need to inherit Initilizable since it is already inherited in ERC721Upgradable.

Referenced Code:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/25aabd286e002a1526c345c8db259d57bdf0ad28/contracts/token/ERC721/ERC721Upgradeable.sol#L20

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/25aabd286e002a1526c345c8db259d57bdf0ad28/contracts/access/OwnableUpgradeable.sol#L21

[10] Function has visibility modifier internal but is not used internally

tokenToSeed( ) in Generative.sol has visibility modifier internal but is not used internally

[11] Return values are not used

The return values in createOpenEdition( ), createLPDASale( ), and createFixedSale( ), and createContract( ) are not used. If the return value is meant to be used to keep track of the new clone's address, that information can be found in the emitted event.

Referenced Code:

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher721Factory.sol#L31 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPriceFactory.sol#L29 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDAFactory.sol#L29 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEditionFactory.sol#L29