Constant variable can be immutable (UFactoryProvider.sol)

[code423n4]

Handle

ye0lde

Vulnerability details

Impact

Changing the variable from constant to immutable will reduce keccak operations and save gas.

Proof of Concept

The variable that can be changed from constant to immutable is here:
https://github.com/code-423n4/2021-12-perennial/blob/fd7c38823833a51ae0c6ae3856a3d93a7309c0e4/protocol/contracts/factory/UFactoryProvider.sol#L23

A previous finding with additional explanation and a pointer to the Ethereum/solidity issue is here:
code-423n4/2021-10-slingshot-findings#3

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Change the constant variable to immutable.

[kbrizzle]

Support for this was added in Solidity v0.6.12.

[GalloDaSballo]

I've built a very simple test:

// SPDX-License-Identifier: MIT

pragma solidity 0.8.10;

contract Keccak8 {
    bytes32 private constant FACTORY_SLOT = keccak256("equilibria.perennial.UFactoryProvider.factory");

    function factory() public view virtual returns (address result) {
        bytes32 slot = FACTORY_SLOT;
        assembly {
            result := sload(slot)
        }
    }
}

Vs

// SPDX-License-Identifier: MIT

pragma solidity 0.8.10;

contract KeccakImmutable {
    bytes32 private immutable factory;    
    constructor(){
        factory = keccak256("equilibria.perennial.UFactoryProvider.factory");

    }
}

And I'm getting

Keccak 8
108949

Keccak 8 immutable
67860

So arguably immutable is way cheaper

This without optimizer

If I do it with the optimizer:

Keccak 8
88235

Keccak 8 Immutable
67786

While this may not be enough, I think the finding is valid

[kbrizzle]

@GalloDaSballo I dug into this as well, because I was really interested in seeing how this works. Here's what I'm getting which is a bit different than your result:

Constant

pragma solidity 0.8.10;

contract StorageTest {
    event Value(address value);

    bytes32 private constant FACTORY_SLOT = keccak256("equilibria.perennial.UFactoryProvider.factory");

    function factory() public view virtual returns (address result) {
        bytes32 slot = FACTORY_SLOT;
        assembly {
            result := sload(slot)
        }
    }

    function testEmit() external {
        emit Value(factory());
    }
}

Immutable

pragma solidity 0.8.10;

contract StorageTest2 {
    event Value(address value);

    bytes32 private immutable FACTORY_SLOT;

    constructor() {
        FACTORY_SLOT = keccak256("equilibria.perennial.UFactoryProvider.factory");
    }

    function factory() public view virtual returns (address result) {
        bytes32 slot = FACTORY_SLOT;
        assembly {
            result := sload(slot)
        }
    }

    function testEmit() external {
        emit Value(factory());
    }
}

Using Remix I'm getting the gas costs:

Contract	Deploy	Read (testEmit())
Constant	129943	24596
Immutable	130400	24596

So given this I think it's strictly better to using immutable?

Either way, and I apologize for not including this in the original message, Uxxx contracts in our repo are specifically designed to be used with upgradeable contracts, so they cannot use constructors and therefore immutable data types.

[GalloDaSballo]

Interesting that you get such different values.

I think the proper way would be to setup an hardhat repo and run it a while.

That said, given that the contracts are meant to be upgradeable, you can't use a constructor so the finding is invalid