Escher contest - yellowBirdy's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L107-L112 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L89-L94 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L120-L123 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L81-L88

Vulnerability details

Impact

All three sale contracts rely on auto sending protocolFee and sales proceeds on auction end. The sending algorithm is the same regardless of different sales end conditions and mechanisms, namely

1. fee is being sent to the receiver defined in the Factory ISaleFactory(factory).feeReceiver().transfer(fee);
2. remaining proceeds are sent to sales beneficiary aka saleReceiver sale.saleReceiver.transfer(totalSale - fee); OR selfdestruct(_sale.saleReceiver);

If any of the transfer calls reverts the other won't be executed either. Most significantly if the fee can't be transferred the auction beneficiary will not receive the proceeds. There are two scenarios when this may happen:

1. The feeReceiver is set to an address of a contract with no capacity to receive ETH
2. (Potential) If opcode prices get rebalanced in the future, transfer may fail for contracts that used to work read more

Proof of Concept

Imagine test below, in the context of the provided forge test suite. where in the setUp() we are setting the fee receiver to an unreceptive contracts address. As a consequence test_WhenMintsOut_Buy() passes by reverting the sale ending tx while the beneficiaries balances remain the same.

contract Unreceptive {
    receive() external payable {revert("Don't want your money!");}
}

contract FixedPriceTest is FixedPriceBaseTest {
    FixedPrice public sale;
    Unreceptive private unreceptiveFeeReceiver;
    event End(FixedPrice.Sale _saleInfo);

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

    function setUp() public override {
        super.setUp();
        // change feeReceiver to unreceptive contract
               unreceptiveFeeReceiver = new Unreceptive();
        fixedSales.setFeeReceiver(payable(unreceptiveFeeReceiver));
    }

    function test_Buy() 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));
        // lets buy some NFTs
        sale.buy{value: 1 ether}(1);
        assertEq(address(sale).balance, 1 ether);
    }

    function test_WhenMintsOut_Buy() public {
        test_Buy();
        uint256 cached_balance = address(this).balance - 9 ether;
        // set sale state to be emitted
        fixedSale.currentId = uint48(10);
        vm.expectEmit(true, true, false, true);
        emit End(fixedSale);
        vm.expectRevert("Don't want your money!");
        sale.buy{value: 9 ether}(9);
        assertEq(address(69).balance, 0);
        assertEq(address(unreceptiveFeeReceiver).balance, 0 );
    }
}

Tools Used

Forge

Recommended Mitigation Steps

1: Minimum - replace transfer with call{value:amount}("") so the sales beneficiary won't be affected in case of failure 2: optimal: refactor the ending mechanism such that

1. finished condition of sale is represented as a state var eg isFinished, so is the feeAmount
2. Auction proceeds are claimable following withdraw pattern
3. (optional) Protocol Fee is also claimble by the withdraw pattern

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L107-L112 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L120-L124

Vulnerability details

Impact

FixedSale.sol and OpenEdition.sol use sefldestruct to finalise the sale and deliver the proceeds to the designated beneficiary. selfdesgtruct is, however officially deprecated and will be removed from EVM. Also Escher contracts are non upgradeable. Hence future sales created with these won’t be able to conclude and beneficiaries won’t be able to retrieve funds.

Proof of Concept

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-6049.md

Tools Used

NA

Recommended Mitigation Steps

Use address.call{value: amount}("") instead of selfdestruct to execute the transfer. It could, also be beneficial to use the 'withdraw' pattern instead of auto sending the proceeds in the sales _end() function.