Users can lose funds if they call `buy` with `_amount` larger than type(uint48).max.

[code423n4]

Lines of code

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

Vulnerability details

Impact

The function buy take amount of type uint256 as input. amount is used to check if msg.value is correct depending on the sale price as follows: require(_amount * sale_.price == msg.value, "WRONG PRICE"); but is not casted to uint48 as done to calculate the newId: uint48 newId = uint48(_amount) + sale_.currentId;.

Therefore if the input _amount is greater than type(uint48).max, users will pay a large _amount of NFT but will only get _amount modulo type(uint48).max.

Proof of Concept

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

Foundry code to run with forge test --match-contract "Bug2" --match-test "BugCast" -vvvvv:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {EscherTest} from "./utils/EscherTest.sol";
import {FixedPriceFactory, FixedPrice} from "src/minters/FixedPriceFactory.sol";

contract FixedPriceBaseTest is EscherTest {
    FixedPrice.Sale public fixedSale;
    FixedPriceFactory public fixedSales;

    function setUp() public virtual override {
        super.setUp();
        fixedSales = new FixedPriceFactory();
        fixedSale = FixedPrice.Sale({
            currentId: uint48(0),
            finalId: uint48(281474976710656 - 1),
            edition: address(edition),
            price: uint96(uint256(1 wei)),
            saleReceiver: payable(address(69)),
            startTime: uint96(block.timestamp)
        });
    }

}
contract Bug2 is FixedPriceBaseTest {
    FixedPrice public sale;
    event End(FixedPrice.Sale _saleInfo);

    mapping(uint256 => address) ownersOf;
    uint256 startId;
    uint256 finalId;
    uint256 currentId;

    function setUp() public override {
        super.setUp();
    }

    function testConsole() public {
        console.log("getPrice",sale.getPrice());
        // console.log("startTime",sale.startTime());
        // console.log("editionContract",sale.editionContract());
        console.log("available",sale.available());
        console.log("address(sale).balance", address(sale).balance);
    }

    function test_BugCast() public {
        // make the fixed price sale
        sale = FixedPrice(fixedSales.createFixedSale(fixedSale));
        // authorize the fixed price sale to mint tokens
        edition.grantRole(edition.MINTER_ROLE(), address(sale));
        testConsole();
        // lets buy some NFTs
        sale.buy{value: 281474976710656 wei}(281474976710656);
        assertEq(address(sale).balance, 281474976710656 wei);
        testConsole();
    }

}

Tools Used

Manual review.

Recommended Mitigation Steps

I recommend to cast _amount to uint48 earlier in the function.

[c4-judge]

berndartmueller marked the issue as duplicate of #369

[c4-judge]

berndartmueller changed the severity to 2 (Med Risk)

[c4-judge]

berndartmueller marked the issue as satisfactory