Escher contest - ladboy233's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L124 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#L63 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L58

Vulnerability details

Impact

Unexpected Overflow and underflow arithmetic operation operation can revert transaction and DOS the contract

Proof of Concept

Unexpected Overflow and underflow arithmetic operation operation can revert transaction and DOS the contract

There is one very problematic underflow in LPDA.sol

/// @notice the price of the sale
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);
}

note the line:

temp.startPrice - (temp.dropPerSecond * timeElapsed);

if the seller set the startPrice to 1 ETH, and dropPerSecond 0.01 ETH and timeElapsed is 200 second

If the first 100 seconds, the getPrice can return a value because 1 - 100 * 0.01 > 0, User can buy using the fund.

then after 100 seconds, the getPrice revert in underflow because 101 * 0.01 > 100,

Then getPrice revert, which also revert buy and refund because these two function both use getPrice()

/// @notice allow a buyer to get a refund on the current price difference
function refund() public {
	Receipt memory r = receipts[msg.sender];
	uint80 price = uint80(getPrice()) * r.amount;
	uint80 owed = r.balance - price;
	require(owed > 0, "NOTHING TO REFUND");
	receipts[msg.sender].balance = price;
	payable(msg.sender).transfer(owed);
}

and

function buy(uint256 _amount) external payable {

	uint48 amount = uint48(_amount);
	Sale memory temp = sale;
	IEscher721 nft = IEscher721(temp.edition);
	require(block.timestamp >= temp.startTime, "TOO SOON");
	uint256 price = getPrice();

the issue is the seller may not aware of this potential underflow error and think the setting is find and the user can buy for a while as long as temp.dropPerSecond * timeElapsed > temp.startPrice then suddenly underflow happens and the contract is DOSed.

In terms of the overflow:

/// @notice allow a buyer to get a refund on the current price difference
function refund() public {
	Receipt memory r = receipts[msg.sender];
	uint80 price = uint80(getPrice()) * r.amount;
	uint80 owed = r.balance - price;
	require(owed > 0, "NOTHING TO REFUND");
	receipts[msg.sender].balance = price;
	payable(msg.sender).transfer(owed);
}

note the line:

uint80 price = uint80(getPrice()) * r.amount;

a uint256 price can be downcasted to uint80, but max uint80 * a value clearly overflow the uint80 price,

if we run test in LPDA.t.sol

function test_price_conversion() public {

	uint48 amount = 10;
	uint80 price = type(uint80).max * amount;
	console.log(price);

}

We run:

forge test -vv --match test_price_conversion

We are getting:

Failing tests:
Encountered 1 failing test in test/LPDA.t.sol:LPDATest
[FAIL. Reason: Arithmetic over/underflow] test_price_conversion() (gas: 384)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project handle arithemic operation carefully:

First, I think it is easier to just use uint256, then when substrating, for example a - b, add sanity check to make sure a > b.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPriceFactory.sol#L12 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L109 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L85 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDAFactory.sol#L12 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L92 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEditionFactory.sol#L12

Vulnerability details

Impact

Use payable(address).to to send ETH instead of payable(address).transfer

Proof of Concept

In the current implemetation in FixedPrice.sol, OpenEdition.sol and LPDA.sol,

after the auction is completed, 5% of the fee is sent to the fee receiver sent in the FixedPriceFactory.sol / OpenEditionFactory.sol and LPDAFactory.sol

Let us use FixedPriceFactory.sol as an example (the OpenEdition.sol and LPDA.sol has the same issue.)

contract FixedPriceFactory is Ownable {
    using Clones for address;

    address payable public feeReceiver;

and the feeReceiver can be set in the constructor

constructor() Ownable() {
	implementation = address(new FixedPrice());
	feeReceiver = payable(msg.sender);
}

or via the privileged function:

/// @notice set the fee receiver for fixed price editions
/// @param fees the address to receive fees
function setFeeReceiver(address payable fees) public onlyOwner {
	feeReceiver = fees;
}

then after the auction is completed, the fee receiver received the fee via:

ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

which equals to:

payable(address).transfer

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

transfer use a hardcoded gas limit 2300, but the gas cost can and will change and can break the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Use (bool sent,memory data) = _to.call{value: msg.value}("") instead of the call and use reentrancy guard with avoid reentrancy.

Lines of code

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

Vulnerability details

Impact

Lack of endtime in FixedPrice.sol sale setting, if the item never sold out, the ETH is locked in the contract

Proof of Concept

The expected business requirement is that after the sales finish, 5% of the sales goes to the fee receiver, the rest goes to the Saler.

In the current implemetation in FixedPrice.sol, if all item sold out, the above ETH payment mechanism is triggered.

How do we know it is sold out in the code?

The start nft id and final id is set in the sale setting:

struct Sale {
	// slot 1
	uint48 currentId;
	uint48 finalId;
	address edition;
	// slot 2
	uint96 price;
	address payable saleReceiver;
	// slot 3
	uint96 startTime;
}

If the newId == the final id, the _end function is triggered, which forward 5% of the fee to fee receiver and the rest ETH to salers.

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

the code in the _end is:

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

In we look into the setting in LPDA.sol and OpenEdition.sol

For LPDA.sol, the sale setting is

struct Sale {
	// slot 1
	uint48 currentId;
	uint48 finalId;
	address edition;
	// slot 2
	uint80 startPrice;
	uint80 finalPrice;
	uint80 dropPerSecond;
	// slot 3
	uint96 endTime;
	address payable saleReceiver;
	// slot 4
	uint96 startTime;
}

For OpenEdition, the setting is:

struct Sale {
	// slot 1
	uint72 price;
	uint24 currentId;
	address edition;
	// slot 2
	uint96 startTime;
	address payable saleReceiver;
	// slot 3
	uint96 endTime;
}

In OpenEdition implementation, if the block.timestamp pass the endTime, the _end can be called to transfer the ETH out by calling finalize.

however, because of the Lack of endtime in FixedPrice.sol sale setting, if the item never sold out, the ETH is locked in the contract.

For example, saler set start id to 0 and end id to 10 and the price is 1 ETH.

5 People bought the NFT, 5 ETH is in the contract, but no one is willing to buy now, then the 5 ETH is locked in the FixedPrice.sol contract and neither fee receiver nor the saler is capable of getting this 5 ETH sales revenue.

This issue exists in LPDA.sol as well, although the sale setting as end time, the code requires the item to be sold in order to transfer the sell reveune out, which can lock the ETH in the LPDA.sol

emit Buy(msg.sender, amount, msg.value, temp);

if (newId == temp.finalId) {
	sale.finalPrice = uint80(price);
	uint256 totalSale = price * amountSold;
	uint256 fee = totalSale / 20;
	ISaleFactory(factory).feeReceiver().transfer(fee);
	temp.saleReceiver.transfer(totalSale - fee);
	_end();
}

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project also endTime in the FixedPrice.sol setting to make sure after the endTime, whether the item is sold out or not, the saler can receive the sales revenue to avoid fund lock.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L71 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L82 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/FixedPrice.sol#L62

Vulnerability details

Impact

Unsafe downcasting operation transcate user's input.

Proof of Concept

There are a few unsafe downcasting operation that transcate user's input. The impact can be severe or minial.

In FixedPrice.sol,

/// @notice buy from a fixed price sale after the sale starts
/// @param _amount the amount of editions to buy
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);
}

the amount is unsafely downcasted from uint256 to uint48, note the code:

require(_amount * sale_.price == msg.value, "WRONG PRICE");
uint48 newId = uint48(_amount) + sale_.currentId;

the upper limit for uint48 is 281474976710655,

if user wants to buy more than 281474976710655 amount of nft and pay the 281474976710655 * sale price amount, the user can only receive 281474976710655 amount of nft because of the downcasting.

In LPDA.sol, we are unsafely downcasting the price in the buy function

receipts[msg.sender].amount += amount;
receipts[msg.sender].balance += uint80(msg.value);

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

sale.currentId = newId;

emit Buy(msg.sender, amount, msg.value, temp);

if (newId == temp.finalId) {
	sale.finalPrice = uint80(price);
	uint256 totalSale = price * amountSold;
	uint256 fee = totalSale / 20;
	ISaleFactory(factory).feeReceiver().transfer(fee);
	temp.saleReceiver.transfer(totalSale - fee);
	_end();
}

note the line:

uint80(msg.value) and uint80(price)

In LPDA.sol, same issue exists in the refund function:

/// @notice allow a buyer to get a refund on the current price difference
function refund() public {
	Receipt memory r = receipts[msg.sender];
	uint80 price = uint80(getPrice()) * r.amount;
	uint80 owed = r.balance - price;
	require(owed > 0, "NOTHING TO REFUND");
	receipts[msg.sender].balance = price;
	payable(msg.sender).transfer(owed);
}

note the downcasting: uint80(getPrice())

this means if the price goes above uint80, it will be wrongly trancated to uint80 for price.

The Downcasting in LPDA.sol is damaging because it tracunated user's fund.

Below is the POC:

add test in LPDA.t.sol

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/test/LPDA.t.sol#L167

    function test_LPDA_downcasting_POC() 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

        uint256 val = uint256(type(uint80).max) + 10 ether;
        console.log('msg.value');
        console.log(val);
        sale.buy{value: val}(1);
    
    }

and import "forge-std/console.sol" in LPDA.sol

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

import "forge-std/console.sol";

and add console.log in LPDA.sol buy function.

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

receipts[msg.sender].amount += amount;
receipts[msg.sender].balance += uint80(msg.value);

console.log("truncated value");
console.log(receipts[msg.sender].balance);

We run our test:

forge test -vv --match test_LPDA_downcasting_POC

the output is:

Running 1 test for test/LPDA.t.sol:LPDATest
[PASS] test_LPDA_downcasting_POC() (gas: 385619)
Logs:
  msg.value
  1208935819614629174706175
  truncated value
  9999999999999999999

Test result: ok. 1 passed; 0 failed; finished in 3.61ms

as we can see, user uses 1208935819614629174706175 to buy in LPDA.sol but the balance is truncated to 9999999999999999999, later user is not able to get the refund they are entitled to because the msg.value is unsafely downcasted.

Also note the downcasting for getPrice in LPDA.sol is also a issue:

the getPrice in LPDA.sol returns a uint256

/// @notice the price of the sale
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);
}

but this is downcasted into uint80 in function buy and refund.

uint80 price = uint80(getPrice()) * r.amount;

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project handle downcasting and use safe casting library to make sure the downcast does not unexpected truncate value.

https://docs.openzeppelin.com/contracts/3.x/api/utils#SafeCast

Floating pragma.

Line Of Code:

All file

Vulnerability and recommended fix

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

We recommend the project use the fix version instead of fixing version.

lack of address(0) validation.

Line Of Code:

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

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

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

Vulnerability and recommended fix

When setting the fee receiver, if the owner set the fee receiver to address(0), the 5% of the fee is lost.

/// @notice set the fee receiver for fixed price editions
/// @param fees the address to receive fees
function setFeeReceiver(address payable fees) public onlyOwner {
	feeReceiver = fees;
}

Lack of two step ownership transfer pattern.

Line Of Code:

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

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

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

Vulnerability and recommended fix

The factory contract inherits ownable from openzepplein.

contract FixedPriceFactory is Ownable {

In the Ownable, the owenr is transferred by calling

function transferOwnership(address newOwner) public virtual onlyOwner {
	require(newOwner != address(0), "Ownable: new owner is the zero address");
	_transferOwnership(newOwner);
}

but we are not sure if the new owner is able to claiming the onlyOwner related function.

We recommend the project implement two step transfer to let the new owner claim the ownership.

Missing Event emission.

Line Of Code:

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

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

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

Vulnerability and recommended fix.

/// @notice set the fee receiver for fixed price editions
/// @param fees the address to receive fees
function setFeeReceiver(address payable fees) public onlyOwner {
	feeReceiver = fees;
}

changing fee receiver changes smart contract states and update the fee receiver address, which should emit event for indexer to capture the state change.

Missing calling _init() function call when using upgradeable contract

Line Of Code:

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

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

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

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher721.sol#L14

Vulnerability and recommended fix.

The Escher721 inherits from ERC2981Upgradeable.

contract Escher721 is
    Initializable,
    ERC721Upgradeable,
    AccessControlUpgradeable,
    ERC2981Upgradeable
{

then the Escher721 should call __ERC2981_init() in initialize function.

    function initialize(
        address _creator,
        address _uri,
        string memory _name,
        string memory _symbol
    ) public initializer {
        __ERC721_init(_name, _symbol);
        __AccessControl_init();

        tokenUriDelegate = _uri;

        _grantRole(DEFAULT_ADMIN_ROLE, _creator);
        _grantRole(MINTER_ROLE, _creator);
        _grantRole(URI_SETTER_ROLE, _creator);
    }

OpenEdition.sol, LPDA.sol and FixedPrice.sol inherits from OwnableUpgradeable,

then the contract should call in the initializer function to make sure the owner is properly set.

__Ownable_init()

Lack of timestamp input validation when setting the sale parameter.

Line Of Code:

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

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

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

Vulnerability and recommended fix.

When creating sales using FixedPrice.sol and OpenEdition.sol and LPDA.sol

Using needs to set the start time and end time

struct Sale {
	// slot 1
	uint72 price;
	uint24 currentId;
	address edition;
	// slot 2
	uint96 startTime;
	address payable saleReceiver;
	// slot 3
	uint96 endTime;
}

The code should check if the block.timestamp < startTime < endTime.

start time and end time should not be in the past and start time should be earlier than endTime, otherwise, the buy function would not work properly.