Escher contest - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDAFactory.sol#L29-L42

Vulnerability details

Impact

LPDAFactory should not allow to create LPDA where startPrice < (end - start) * dropRate. Otherwise it's possible that LPDA contract will be out of service, because getPrice will always revert.

Proof of Concept

Let's look at LPDA.getPrice function. https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L117-L125

    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);
    }

The price is decreasing during the time and decreased amount depends on temp.dropPerSecond and timeElapsed.

In case if temp.dropPerSecond * timeElapsed > temp.startPrice then getPrice function will be always reverting, because of underflow error. As result it will not be possible to finalize sales and seller will never receive funds for tokens that were sold.

This is because, LPDAFactory.createLPDASale doesn't check that.

Tools Used

VsCode

Recommended Mitigation Steps

Check that startPrice >= (end - start) * dropRate in LPDAFactory.createLPDASale function. Otherwise revert.

Lines of code

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

Vulnerability details

Impact

In different contracts protocol uses transfer function to send ethers to recipient. But in case if recipient is contract and his receive/fallback function needs more than 2300 gas(that is send with transfer), then the function will revert.

Proof of Concept

When FixedPrice contract has sold all tokens then _end() function is called to send payment. https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L107-L111

    function _end(Sale memory _sale) internal {
        emit End(_sale);
        ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
        selfdestruct(_sale.saleReceiver);
    }

In case if feeReceiver is contract and his receive/fallback function needs more than 2300 gas to finish then the function will revert and as result it will not be possible to finalize sales and last token will not be sold.

Same approach is used in another contracts. For example in LPDA when user wants to get refund, transfer is used as well.

Tools Used

VsCode

Recommended Mitigation Steps

Use call function instead of transfer.

Lines of code

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

Vulnerability details

Impact

In case if all tokens are not sold in FixedPrice contract then seller can't withdraw received amount from sales.

Proof of Concept

FixedPrice.buy function allows someone to buy some amount of tokens. If all tokens that seller wanted to sell are sold, then sales is over and payment is sent to fee receiver and seller. This is the only option to finish sales after it started.

So in case if not all tokens were sold and noone else wants to buy, seller can't withdraw funds that were paid for already sold tokens, this funds are stucked inside FixedPrice contract.

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

    function buy(uint256 _amount) external payable {
        Sale memory sale_ = sale;
        IEscher721 nft = IEscher721(sale_.edition);
        require(block.timestamp >= sale_.startTime, "TOO SOON");
        require(_amount * sale_.price == msg.value, "WRONG PRICE");
        uint48 newId = uint48(_amount) + sale_.currentId;
        require(newId <= sale_.finalId, "TOO MANY");


        for (uint48 x = sale_.currentId + 1; x <= newId; x++) {
            nft.mint(msg.sender, x);
        }


        sale.currentId = newId;


        emit Buy(msg.sender, _amount, msg.value, sale);


        if (newId == sale_.finalId) _end(sale);
    }

Tools Used

VsCode

Recommended Mitigation Steps

Add ability for seller to withdraw already paid amount or add salesEnd param after which seller can end sales.

Lines of code

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

Vulnerability details

Impact

In case if payment of user were more than uint80.max value, then the balance stored to receipt will be truncated to uint80.max. As result user will not be able to refund all that he overpaid.

Proof of Concept

When user wants to buy tokens he calls LPDA.buy and provides ether with function. Later the amount that he paid is stored in the receipt. It will be used when the price will decrease and user will be able to refund overpaid tokens.

The problem is that msg.value here is unsafely casted to uint80. That means that if user paid more than uint80.max value, then delta amount is lost as it's not stored into receipts[msg.sender].balance. receipts[msg.sender].balance += uint80(msg.value);

So later he will not be able to withdraw all overpaid amount using refund as it uses his balance.

Tools Used

Recommended Mitigation Steps

When casting check that msg.value is less than uint80.max

Lines of code

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

Vulnerability details

Impact

In different contracts protocol uses ERC721 mint function to create new token for address instead of safeMint. Because of that it's possible to mint token to the contract that doesn't support ERC721 and token will be stucked there,

Proof of Concept

Inside FixedPrice.buy function new tokens are minted using mint function. In case if msg.sender is contract and it doesn't support ERC721, using safeMint is better. Otherwise token can be stucked inside contract.

Tools Used

VsCode

Recommended Mitigation Steps

Use safeMint that will check that recipient supports ERC721 tokens.