Escher contest - sorrynotsorry's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDAFactory.sol#L29-L42 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L117-L125

Vulnerability details

Impact

dropPerSecond is used for the price drop of the auction per second. However it was not validated comparing to the price and the time parameters. dropPerSecond should ideally be equal to (startPrice - finalPrice) / (endTime - startTime) but this validation was not done in both LPDAFactory and LPDA contracts.

There are 2 scenarios on wrong setting of the dropPerSecond parameter.

1. When the parameter dropPerSecond is greater than it should be; Since it will drop the price faster than it should be, there will be a point where the price will be less than the finalPrice and the buyers can buy the NFT's cheaper than the finalPrice. And the difference between the finalPrice and falsePrice will depend on how high the parameter is set. If it's a steep value, the user might even loss the NFT to the dust values since the returned price was never checked being less than the finalPrice. But once the getPrice function's (temp.dropPerSecond * timeElapsed) threshold is passed the getPrice function will also panic due to (temp.dropPerSecond * timeElapsed) will be greater than temp.startPrice

2. When the parameter dropPerSecond is less than it should be; Since it will drop the price slower than supposed to be, the finalPrice will not be reached between startTime and endTime. The NFT owner might not sell his/her NFT and there might be an other contract to be created via createLPDASale.

Proof of Concept

Please add the snipped to LPDA.t.sol for the test which buys NFT to a lower price than the finalPrice.

Change the setup() as;

    function setUp() public virtual override {
        super.setUp();
        lpdaSales = new LPDAFactory();
        // 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.9 ether)), //@audit-info finalPrice is changed for the test
            dropPerSecond: uint80(uint256(0.11 ether) / 1 days), //@audit-info dropPerSecond is increased
            startTime: uint96(block.timestamp),
            saleReceiver: payable(address(69)),
            endTime: uint96(block.timestamp + 1 days)
        });
    }

Please add the test function as below;

function test_LPDA_buy_lower() public {
        // make the lpda sales contract
        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(block.timestamp + 1 days);
        assertLt(sale.getPrice(), lpdaSale.finalPrice);//@audit-info the price returns less than the finalPrice

        // buy the rest
        // this will auto end the sale
        sale.buy{value: uint256((0.89 ether + lpdaSale.dropPerSecond) * 9)}(9); //@audit-info each NFT's are bought less than the final price
    }

The result at the console is;

[PASS] test_LPDA_buy_lower() (gas: 689791)

Sale is created - No validation for dropPerSecond

    function createLPDASale(LPDA.Sale calldata sale) external returns (address clone) {
        require(IEscher721(sale.edition).hasRole(bytes32(0x00), msg.sender), "NOT AUTHORIZED");
        require(sale.saleReceiver != address(0), "INVALID SALE RECEIVER");
        require(sale.startTime >= block.timestamp, "INVALID START TIME");
        require(sale.endTime > sale.startTime, "INVALID END TIME");
        require(sale.finalId > sale.currentId, "INVALID FINAL ID");
        require(sale.startPrice > 0, "INVALID START PRICE");
        require(sale.dropPerSecond > 0, "INVALID DROP PER SECOND"); //@audit-info The only validation
        clone = implementation.clone();
        LPDA(clone).initialize(sale);
        emit NewLPDAContract(msg.sender, sale.edition, clone, sale);
    }

Permalink

getPrice function directly uses the set parameter and the returned price was never checked being less than the finalPrice.

    function getPrice() public view returns (uint256) {
        Sale memory temp = sale;
        (uint256 start, uint256 end) = (temp.startTime, temp.endTime);
        if (block.timestamp < start) return type(uint256).max;
        if (temp.currentId == temp.finalId) return temp.finalPrice;
        uint256 timeElapsed = end > block.timestamp ? block.timestamp - start : end - start;
        return temp.startPrice - (temp.dropPerSecond * timeElapsed);
    }

Permalink

Tools Used

Manual Review

Recommended Mitigation Steps

Validate dropPerSecond