Escher contest - cryptonue's results

Lines of code

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

Vulnerability details

Impact

The buy() function on Last Price Dutch Auction Sale doesn't check if auction is ended, may lead to user loss asset if user call with amount 0 with msg.value > 0

Proof of Concept

Ideally if the max id (finalId) is reached, then the auction will end, so no user can call buy(). If we see the FixedPrice and OpenEdition there is a selfdestruct when _end(), so user can't call buy() again.

But for LPDA, the _end() doesn't have this selfdestruct, thus there is a potential call user calling the buy().

The buy() function only have one check require(newId <= temp.finalId, "TOO MANY"); to implicitly says it's already fulfilled the max id.

There is a scenario where user call buy() function with providing the amount with 0. If this user fill the amount with 0 and send msg.value, the buy() will success since the require(msg.value >= amount * price, "WRONG PRICE"); and require(newId <= temp.finalId, "TOO MANY"); will passed. At the end, the user will not get any amount minted but their msg.value can't be refund(), because it will be transferred to saleReceiver and feeReceiver.

Even though user is hardly input 0 as the amount, but still this can be an issue if for example the Frontend (UI) wrongly input the amount value to 0, or any misleading action. This user lost of asset (msg.value) usually categorized as high issue, but since this is need additional factor, it suitable with medium.

File: LPDA.sol
58:     function buy(uint256 _amount) external payable {
59:         uint48 amount = uint48(_amount);
60:         Sale memory temp = sale;
61:         IEscher721 nft = IEscher721(temp.edition);
62:         require(block.timestamp >= temp.startTime, "TOO SOON");
63:         uint256 price = getPrice();
64:         require(msg.value >= amount * price, "WRONG PRICE");
65: 
66:         amountSold += amount;
67:         uint48 newId = amount + temp.currentId;
68:         require(newId <= temp.finalId, "TOO MANY");
69: 
70:         receipts[msg.sender].amount += amount;
71:         receipts[msg.sender].balance += uint80(msg.value);
72: 
73:         for (uint256 x = temp.currentId + 1; x <= newId; x++) {
74:             nft.mint(msg.sender, x);
75:         }
76: 
77:         sale.currentId = newId;
78: 
79:         emit Buy(msg.sender, amount, msg.value, temp);
80: 
81:         if (newId == temp.finalId) {
82:             sale.finalPrice = uint80(price);
83:             uint256 totalSale = price * amountSold;
84:             uint256 fee = totalSale / 20;
85:             ISaleFactory(factory).feeReceiver().transfer(fee);
86:             temp.saleReceiver.transfer(totalSale - fee);
87:             _end();
88:         }
89:     }

Tools Used

Manual analysis

Recommended Mitigation Steps

Add new state variable to check if the auction is ended, and put the guard in buy() function, example:

bool isAuctionEnded;

function _end() internal {
       emit End(sale);
       isAuctionEnded = true;
}

function buy(uint256 _amount) external payable {
    require(!isAuctionEnded, "AUCTION ENDED");
    ...
}

Lines of code

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

Vulnerability details

Impact

call() should be used instead of transfer() on an address payable

Proof of Concept

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

1. The claimer smart contract does not implement a payable function.
2. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
3. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300. Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

File: FixedPrice.sol
109:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

File: LPDA.sol
85:             ISaleFactory(factory).feeReceiver().transfer(fee);
86:             temp.saleReceiver.transfer(totalSale - fee);
...
105:         payable(msg.sender).transfer(owed);

File: OpenEdition.sol
92:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

Tools Used

Manual Analysis

Recommended Mitigation Steps

Recommend using call() instead of transfer()