Escher contest - ajtra's results

Lines of code

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

Vulnerability details

Impact

The use of transfer() will revert the transaction when the receiver is a contract and when one of the following conditions is met:

• The contract does not have a payable callback
• The contract's payable callback spends more than 2300 gas (which is only enough to emit something)
• The contract is called through a proxy which itself uses up the 2300 gas

Proof of Concept

Step 1. Create a contract without a payable callback

contract ReceiverContract {
}

Step 2. Create a LPDA.Sale with the saleReceiver parameter setted up to the example contract ReceiverContract

contract LPDABase is EscherTest {
    LPDAFactory public lpdaSales;
    LPDA.Sale public lpdaSale;
	ReceiverContract public receiverTest;
	
    function setUp() public virtual override {
        super.setUp();
        lpdaSales = new LPDAFactory();
	receiverTest = new ReceiverContract();
        // set up a LPDA Sale
        lpdaSale = LPDA.Sale({
            currentId: uint48(0),
            finalId: uint48(1),
            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(receiverTest)),
            endTime: uint96(block.timestamp + 1 days)
        });
    }
}

Step 3. Create a situation where the function buy execute the sentence temp.saleReceiver.transfer(totalSale - fee); in line 86 function buy

contract LDPAPayableTest is LPDABase {
    LPDA public sale;
    function test_pay_LPDA() 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
        sale.buy{value: 1 ether}(1);
    }
}

Step 4. The transaction will revert. [FAIL. Reason: EvmError: Revert] test_pay_LPDA() (gas: 400569)

Tools Used

forge test -vvvvv

Recommended Mitigation Steps

Use call() instead of transfer(). Add reentrancy guard to avoid reentrancy attack.

Lines of code

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

Vulnerability details

Impact

The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:

• The contract does not have a payable callback
• The contract's payable callback spends more than 2300 gas (which is only enough to emit something)
• The contract is called through a proxy which itself uses up the 2300 gas

If a user falls into one of the above categories, they'll be unable to receive funds from the vault in a refund. Inaccessible funds means loss of funds, which is Medium severity.

Proof of Concept

refund() use payable.transfer(). While use msg.sender, the funds are tied to the address that deposited them (line 100), and there is no mechanism to change the owner of the funds to an alternate address.

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

Tools Used

Manual inspection

Recommended Mitigation Steps

Use address.call{value:x}() instead

#Index

Low

1. L01 - initialize() function can be called by anybody

Non Critical

2. NC01 - Floating pragma
3. NC02 - Struct definition should be located before state variables
4. NC03 - Include return parameters in NatSpec comments
5. NC04 - Missing NatSpec File
6. NC05 - Empty block should be removed
7. NC06 - Integer overflow by unsafe casting

L01 - initialize() function can be called by anybody

Description

initialize() function can be called anybody when the contract is not initialized.

More importantly, if someone else runs this function, they will have full authority because of the __AccessControl_init() function. Also, there is no 0 address check in the address arguments of the initialize() function, which must be defined.

Lines in the code

Escher.sol#L32-L46

NC01 - Floating pragma

Description

Contracts have the pragma solidity directive ^0.8.17. It is recommended to specify a fixed compiler version to ensure that the bytecode produced does not vary between builds. This is especially important if you rely on bytecode-level verification of the code.

Mitigation

Lock the pragma.

Lines in the code

Unique.sol#L2 Base.sol#L2 Generative.sol#L2 Escher721Factory.sol#L2 FixedPriceFactory.sol#L2 OpenEditionFactory.sol#L2 LPDAFactory.sol#L2 Escher.sol#L2 Escher721.sol#L2 FixedPrice.sol#L2 OpenEdition.sol#L2 LPDA.sol#L2

NC02 - Struct definition should be located before state variables

Mitigation

Move structs before the state variables

Lines in the code

LPDA.sol#L13 LPDA.sol#L29 FixedPrice.sol#L14 OpenEdition.sol#L13

NC03 - Include return parameters in NatSpec comments

Description

According with solidity documentation link If return parameters are declared must be added to NatSpec. Some code analysis use NatSpec for analysis the code.

Mitigation

Add /// @return to function with return parameter

Lines in the code

Base.sol#L14 Generative.sol#L27 Unique.sol#L10 OpenEdition.sol#L97 OpenEdition.sol#L102 OpenEdition.sol#L107 OpenEdition.sol#L112 OpenEdition.sol#L116 OpenEditionFactory.sol#L29 LPDA.sol#L117 LPDA.sol#L128 LPDA.sol#L133 LPDA.sol#L138 LPDA.sol#L142 LPDAFactory.sol#L29 FixedPriceFactory.sol#L29 Escher721Factory.sol#L27 Escher721.sol#L78 Escher721.sol#L84 Escher.sol#L31

NC04 - Missing NatSpec File

Lines in the code

Unique.sol Base.sol Generative.sol Escher721Factory.sol FixedPriceFactory.sol OpenEditionFactory.sol LPDAFactory.sol Escher.sol Escher721.sol FixedPrice.sol OpenEdition.sol LPDA.sol

NC05 - Empty block should be removed

Description

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

Lines in the code

Escher721.sol#L25

NC06 - Integer overflow by unsafe casting

Description

Solidity versions higher 0.8 don't prevent integet overflow for casting operations just in mathematical operations.

Mitigation

Use a safeCast from OpenZeppelin

Lines in the code

LPDA.sol#L59 LPDA.sol#L82 LPDA.sol#L101 OpenEdition.sol#L58 FixedPrice.sol#L62 Escher.sol#L61 Escher.sol#L63 Escher.sol#L69

Index

1. Optimize LPDA.getPrice()
2. I = I + (-) X is cheaper in gas cost than I += (-=) X
3. Optimize LPDA.buy
4. Unchecked operations in getPrice() when it's not possible to overflow/underflow
5. Use unchecked in for/while loops when it's not possible to overflow

Details

1. Optimize LPDA.getPrice()

Description

It's possible to save gas if move the second if to the beginning. There are situations where that if is true and it's not necessary to assig value to start & end local variables or evaluate the first if (block.timestamp < start).

    function getPrice() public view returns (uint256) {
        Sale memory temp = sale;
+       if (temp.currentId == temp.finalId) return temp.finalPrice;
        (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);
    }

Lines in the code

LPDA.sol#L117-L125

2. I = I + (-) X is cheaper in gas cost than I += (-=) X

Description

When we are working with state variables is cheaper to use I = I + X than I += X.

PoC

Original Code Running 14 tests for test/LPDA.t.sol:LPDATest [PASS] test_Buy() (gas: 381183)

Code after apply changes Running 14 tests for test/LPDA.t.sol:LPDATest [PASS] test_Buy() (gas: 381141)

There is 42 gas saved with this change for each instance. Due to there are 3 instances the gas saved is 126

Lines in the code

LPDA.sol#L66 LPDA.sol#L70 LPDA.sol#L71

3. Optimize LPDA.buy

Description

The declaration of the variable is made at the beginning of the function while its use is made later on. Between the declaration of the variable and its use, there are several requirements that may result in the variable never being used. By moving the declaration after the requires and before it is used, gas can be saved in some circunstances.

    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();
        require(msg.value >= amount * price, "WRONG PRICE");

        amountSold += amount;
        uint48 newId = amount + temp.currentId;
        require(newId <= temp.finalId, "TOO MANY");

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

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

LPDA.sol#L58-L75 FixedPrice.sol#L59-L67

4. Unchecked operations in getPrice() when it's not possible to overflow/underflow

Description

There are code where it's not possible to overflow/undeflow due to a IF or Require statement. In that case, it's possible to save gas adding the unckecked block.

Change proposed

    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;
+       uint256 timeElapsed;
+       unchecked{ timeElapsed = end > block.timestamp ? block.timestamp - start : end - start;}
        return temp.startPrice - (temp.dropPerSecond * timeElapsed);
    }

PoC

Original Code

Code after apply changes

There is 66 of gas saved directly in getPrice due to the change. Due to getPrice is a use in other functions (buy and refund) there is a major gas saved because of each time this functions use getPrice 66 of gas is saved.

Lines in the code

LPDA.sol#L117-L125

5. Use unchecked in for/while loops when it's not possible to overflow

Description

Use unchecked { i++; } or unchecked{ ++i; } when it's not possible to overflow to save gas.

Change proposed

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

PoC

Original code Running 14 tests for test/LPDA.t.sol:LPDATest [PASS] test_Buy() (gas: 381183)

Code after apply changes Running 14 tests for test/LPDA.t.sol:LPDATest [PASS] test_Buy() (gas: 381124)

In the example of execute the test_BUY without and with the change proposed it's possible to have 59 gas saved for instance.

Lines in the code

FixedPrice.sol#L65 OpenEdition.sol#L66 LPDA.sol#L73