Escher contest - 0x1f8b's results

Lines of code

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

Vulnerability details

Impact

In the OpenEdition contract, the user can buy a different amount than expected.

Proof of Concept

The following proof of concept wants to demonstrate how if a user buys (16777215) + 2 with a price of 1, it should cost 16777217 in ether, but instead they will buy only one for one satoshi, which is incorrect. to what was said buy(16777217)

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types. The problem lies in an unsafe cast on uint24 quantity = uint24(_quantity);.

contract POCTest is EscherTest {
    OpenEdition public sale;
    OpenEditionFactory public openSales;
    OpenEdition.Sale public openSale;

    function setUp() public virtual override {
        super.setUp();
        openSales = new OpenEditionFactory();
        openSale = OpenEdition.Sale({
            price: 1,  // <- PRICE 1
            currentId: uint24(1),
            edition: address(edition),
            startTime: uint96(block.timestamp),
            saleReceiver: payable(address(69)),
            endTime: uint96(block.timestamp + 1 days)
        });
    }

    function test_POC() public {
        sale = OpenEdition(openSales.createOpenEdition(openSale));
        edition.grantRole(edition.MINTER_ROLE(), address(sale));
        
        (uint72 price, uint24 currentId, address edition, uint96 startTime, address payable saleReceiver, uint96 endTime) = sale.sale();
        assertEq(currentId, 1);
        assertEq(address(sale).balance, 0);
        
        sale.buy{value: 1}((16777215) + 2);     // Amount overflow
        assertEq(address(sale).balance, 1);
        (price,  currentId, edition,  startTime, saleReceiver, endTime) = sale.sale();
        assertEq(currentId, 2);
    }
}

Affected source code

• OpenEdition.sol:58

Recommended Mitigation Steps

• Use a safeCast from Open Zeppelin.

• Low
  • 1. Improve tokenURI logic
  • 2. Front running in initialize method
  • 3. Owner can steal the fees
  • 4. Contracts without GAP can lead a upgrade disaster
  • 5. Integer overflow by unsafe casting
• Non critical
  • 6. Use abstract for base contracts

Low

1. Improve tokenURI logic

The tokenURI method does not check that _tokenId exists, so it might return uris for invalid tokens. This behavior should be copied from the logic of Open Zeppelin where they check if the token was minted before returning a valid result.

Affected source code:

• Unique.sol:10
• Base.sol:15

2. Front running in initialize method

The methods that inherit the Base contract have a method that is susceptible to front-running attacks, which allows an attacker to obtain ownership and take advantage of the contract as soon as it is deployed, it is convenient to protect this type of initializations making sure that only a specific sender can call said method.

In the case of the 'Generative' contract, the attacker can call setScriptPiece with arbitrary data and exploit or combine other attacks.

Affected source code:

• Base.sol:18
• Unique.sol:7
• Generative.sol:6

3. Owner can steal the fees

If the owner calls cancel, it will not pass any fees to feeReceiver, so it is possible for the owner to avoid this commitment to the fee manager. All this fees will be transfered to saleReceiver instead of the feeReceiver as was agreed during the deployment.

Affected source code:

• OpenEdition.sol:75

4. Contracts without GAP can lead a upgrade disaster

Some contracts do not implement good upgradeable logic.

Upgrading a contract will need a __gap storage in order to avoid storage collisions.

Proxied contracts MUST include an empty reserved space in storage, usually named __gap, in all the inherited contracts.

For example, the contract Governed or GraphUpgradeable are inherited by upgradeable contracts, but these contracts don't have a __gap storage, so if a new variable is created it could lead to storage collisions that will produce a contract disaster, including loss of funds.

Reference:

• https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps
• https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/2cb8996b777060e658e2b8c9b1630313aedb04c0/contracts/token/ERC20/ERC20Upgradeable.sol#L394

Affected source code:

• Unique.sol:7
• Base.sol:7
• Generative.sol:7
• OpenEdition.sol:10

Recommended Mitigation Steps:

• Add uint256[50] private __gap; to all upgradable contracts.

5. Integer overflow by unsafe casting

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Recommendation:

Use a safeCast from Open Zeppelin.

uint80 price = uint80(getPrice()) * r.amount;

Affected source code:

• FixedPrice.sol:62
• LPDA.sol:59
• LPDA.sol:71
• LPDA.sol:82
• LPDA.sol:101
• OpenEdition.sol:58

---

Non critical

6. Use abstract for base contracts

abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• Unique.sol:7
• Base.sol:7
• Generative.sol:7