code-423n4 / 2022-12-escher-findings

0 stars 0 forks source link

FUNDS WILL BE LOCKED IF ```saleReceiver``` DOESN'T IMPLEMENT FALLBACK FUNCTIONS #122

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

If the saleReceiver in LPDA.sol is a contract address without receive() or fallback() functions, ethers will be locked. saleReceiver is not updatable.

The feeReceiver in LPDAFactory.sol, FixedPriceFactory.sol and OpenEditionFactory.sol is also affected, but there is a setter for this address and the user can change it with another one.

Proof of Concept

Test case

// 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 SaleReceiver {
    // empty
}

contract LPDAPush is EscherTest {
    LPDAFactory public lpdaSales;
    LPDA.Sale public lpdaSale;
    SaleReceiver public saleReceiver;
    LPDA public sale;

    function setUp() public virtual override {
        saleReceiver = new SaleReceiver();

        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(saleReceiver)),
            endTime: uint96(block.timestamp + 1 days)
        });
    }

    function test_TransferToContractWithoutReceive() public {
        sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
        // authorize the lpda sale to mint tokens
        edition.grantRole(edition.MINTER_ROLE(), address(sale));

        sale.buy{value: 1 ether}(1);
        assertEq(address(sale).balance, 1 ether);

        vm.warp(block.timestamp + 1 days);
        assertApproxEqRel(sale.getPrice(), 0.9 ether, lpdaSale.dropPerSecond);

        // buy the rest
        // this will auto end the sale
        sale.buy{value: uint256((0.9 ether + lpdaSale.dropPerSecond) * 9)}(9);
    }
}

Failing part of the test result

    │   │   ├─ [24] SaleReceiver::fallback{value: 8550000000000334400}() 
    │   │   │   └─ ← "EvmError: Revert"
    │   │   └─ ← "EvmError: Revert"
    │   └─ ← "EvmError: Revert"
    └─ ← "EvmError: Revert"

Test result: FAILED. 0 passed; 1 failed; finished in 4.94ms

Failing tests:
Encountered 1 failing test in test/LPDADos.t.sol:LPDAPush
[FAIL. Reason: EvmError: Revert] test_TransferToContractWithoutReceive() (gas: 661309)

Tools Used

Foundry

Recommended Mitigation Steps

The recommended way to is to use pull over push pattern. Reference: ConsenSys Development Recommendations. If the pull over push pattern will not be used, there should be a update function for the saleReceiver.

c4-judge commented 1 year ago

berndartmueller marked the issue as unsatisfactory: Overinflated severity