Fractional v2 contest - c3phas's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L172 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L325

Vulnerability details

call() should be used instead of transfer() on address payable

Impact

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

1. The withdrawer smart contract does not implement a payable function.
2. The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
3. The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

To prevent unexpected behavior and potential loss of funds, consider explicitly warning end-users about the mentioned shortcomings to raise awareness before they deposit Ether into the protocol. Additionally, note that the sendValue function available in OpenZeppelin Contract’s Address library can be used to transfer the withdrawn Ether without being limited to 2300 gas units. Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the “Check-effects-interactions” pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. For further reference on why using Solidity’s transfer is no longer recommended, refer to these articles:

• Stop using Solidity’s transfer now
• Reentrancy after Istanbul

Affected code

File: Migration.sol line 172

        payable(msg.sender).transfer(ethAmount);

File: Migration.sol line 325

        payable(msg.sender).transfer(userEth);

Tool

Visual studio code (manual code review)

Recommendation

Use solidity's low level call() function or the sendValue function from openzeppelin

QA

Open Todos

File: MerkleBase.sol line 24

            // TODO: This can be aesthetically simplified with a switch. Not sure it will

Natspec is incomplete

File: FERC1155.sol line 238-248

Missing @return

    /// @notice Sets the token royalties
    /// @param _id Token ID royalties are being updated for
    /// @param _salePrice Sale price to calculate the royalty for
    function royaltyInfo(uint256 _id, uint256 _salePrice)
        external
        view
        returns (address receiver, uint256 royaltyAmount)
    {
        receiver = royaltyAddress[_id];
        royaltyAmount = (_salePrice * royaltyPercent[_id]) / 100;
    }

File: FERC1155.sol line 289-295 Missing @return

    /// @notice Getter for URI of a token type
    /// @param _id ID of the token type
    function uri(uint256 _id)
        public
        view
        override(ERC1155, IFERC1155)
        returns (string memory)

File: FERC1155.sol line 301 Missing @return

    /// @notice Getter for controller account
    function controller() public view returns (address controllerAddress) {

File: FERC1155.sol line 318-323 Missing @return

    /// @dev Computes hash of permit struct
    /// @param _owner Address of the owner of the token type
    /// @param _operator Address of the spender of the token type
    /// @param _id ID of the token type
    /// @param _approved Approval status for the token type
    /// @param _deadline Expiration of the signature

File: FERC1155.sol line 345-349

Missing @return

**    /// @dev Computes hash of permit all struct
    /// @param _owner Address of the owner of the token type
    /// @param _operator Address of the spender of the token type
    /// @param _approved Approval status for the token type
    /// @param _deadline Expiration of the signature**

File: BaseVault.sol line 28-40

Missing @return

    /// @notice Deploys a new Vault and mints initial supply of fractions
    /// @param _fractionSupply Number of NFT Fractions minted to control the vault
    /// @param _modules The list of modules to be installed on the vault
    /// @param _plugins Addresses of plugin contracts
    /// @param _selectors List of function selectors
    /// @param _mintProof List of proofs to execute a mint function
    function deployVault(
        uint256 _fractionSupply,
        address[] calldata _modules,
        address[] calldata _plugins,
        bytes4[] calldata _selectors,
        bytes32[] calldata _mintProof
    ) external returns (address vault) {

Duplicate conditions should be converted to a modifier

File: Vault.sol line 76

        if (owner != msg.sender) revert NotOwner(owner, msg.sender);

The above line(check) has been repeated at least 3 more times in the same file

File: Vault.sol line 87 File: Vault.sol line 94 File: Vault.sol line 102

Missing checks for address(0x0) when assigning values to address state variables

File: Vault.sol line 93-97

    function transferOwnership(address _newOwner) external {
        if (owner != msg.sender) revert NotOwner(owner, msg.sender);
        owner = _newOwner;
        emit TransferOwnership(msg.sender, _newOwner);
    }

The above function does not check for address 0x0 on address _newOwner which might cause transfer of ownership to be assigned to the zero address

File: Supply.sol line 17

        registry = _registry;

File: Supply.sol line 25

        address _registry = registry;

File: Supply.sol line 115

        address _registry = registry;

File: TransferReference.sol line 22

        IERC20(_token).transfer(_to, _value);

File: SupplyReference.sol line 16

        registry = _registry;

File: BaseVault.sol line 25

        registry = _registry;

File: Minter.sol line 18

        supply = _supply;

File: Migration.sol line 58-59

        buyout = payable(_buyout);
        registry = _registry;

File: Buyout.sol line 47-49

        registry = _registry;
        supply = _supply;
        transfer = _transfer;

FINDINGS

Using immutable on variables that are only set in the constructor and never after

File: Migration.sol line 39

    address public registry;

The above is only set in the constructor and never set again

File: Migration.sol line 37

    address payable public buyout;

File: Buyout.sol line 29

    address public registry;

File: Buyout.sol line 31

    address public supply;

File: Buyout.sol line 33

    address public transfer;

File: BaseVault.sol line 19

    address public registry;

Using unchecked blocks to save gas - Increments in for loop can be unchecked ( save 30-40 gas per loop iteration)

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code File: Vault.sol line 78

        for (uint256 i = 0; i < length; i++) {
            methods[_selectors[i]] = _plugins[i];
        }

The above should be modified to:

        for (uint256 i = 0; i < length; {
            methods[_selectors[i]] = _plugins[i];
		unchecked{
			 ++i;
		}
        }

Other Instances to modify File: Vault.sol line 104

        for (uint256 i = 0; i < length; i++) {

see resource

Cache storage values in memory to minimize SLOADs

The code can be optimized by minimizing the number of SLOADs. SLOADs are expensive 100 gas compared to MLOADs/MSTOREs(3gas) Storage value should get cached in memory

NB: Some functions have been truncated where necessary to just show affected parts of the code

Migration.sol.propose() : registry should be cached (saves ~ 92 gas)

File: Migration.sol line 81-95

    function propose(
        address _vault,
        address[] calldata _modules,
        address[] calldata _plugins,
        bytes4[] calldata _selectors,
        uint256 _newFractionSupply,
        uint256 _targetPrice
    ) external {
        // Reverts if address is not a registered vault
        (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
       
        // Initializes migration proposal info
  
        proposal.oldFractionSupply = IVaultRegistry(registry).totalSupply(
            _vault
        );
        proposal.newFractionSupply = _newFractionSupply;
    }

In the above registry should be cached in memory to reduce number of SLOADs.

SLOAD 1: line 81 SLOAD 2: line 95

Migration.sol.migrateFractions() : registry should be cached (saves ~ 372 gas)

File: Migration.sol line 435,467,470

    function migrateFractions(address _vault, uint256 _proposalId) external {
        // Reverts if address is not a registered vault
        (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
       
        // Gets the token and fraction ID of the new vault
        address newVault = migrationInfo[_vault][_proposalId].newVault;
        (address token, uint256 newFractionId) = IVaultRegistry(registry)
            .vaultToToken(newVault);
        // Calculates share amount of fractions for the new vault based on the new total supply
        uint256 newTotalSupply = IVaultRegistry(registry).totalSupply(newVault);
        uint256 shareAmount = (balanceContributedInEth * newTotalSupply) /
            totalInEth;

        );
    }

In the above registry should be cached in memory to reduce number of SLOADs.

SLOAD 1: line 435 SLOAD 2: line 467 SLOAD 3: line 470

Migration.sol.migrateFractions() : registry should be cached (saves ~ 101 gas)

File:Migration.sol line 184,200

    function commit(address _vault, uint256 _proposalId)
        external
        returns (bool started)
    {
        // Reverts if address is not a registered vault
        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
            _vault
        );
        // Calculates current price of the proposal based on total supply
        uint256 currentPrice = _calculateTotal(
            100,
            IVaultRegistry(registry).totalSupply(_vault),
            proposal.totalEth,
            proposal.totalFractions
        );

    }

In the above registry should be cached in memory to reduce number of SLOADs.

SLOAD 1: line 184 SLOAD 2: line 200

Buyout.sol.start() : registry should be cached (saves ~ 83 gas)

File:Buyout.sol line 61-71

    function start(address _vault) external payable {
       
        // Reverts if address is not a registered vault
        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
            _vault
        );
       ...
        // Gets total supply of fractional tokens for the vault
        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
        // Gets total balance of fractional tokens owned by caller
        uint256 depositAmount = IERC1155(token).balanceOf(msg.sender, id);

In the above registry should be cached in memory to reduce number of SLOADs.

SLOAD 1: line 61 and the SLOAD 2: line 71

Buyout.sol.cash() : registry should be cached (saves ~ 84 gas)

File: Buyout.sol line 246-267

    function cash(address _vault, bytes32[] calldata _burnProof) external {
        // Reverts if address is not a registered vault
        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
            _vault
        );
    		...
        // Transfers buyout share amount to caller based on total supply
        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
        uint256 buyoutShare = (tokenBalance * ethBalance) /
            (totalSupply + tokenBalance);
        _sendEthOrWeth(msg.sender, buyoutShare);
        // Emits event for cashing out of buyout pool
        emit Cash(_vault, msg.sender, buyoutShare);
    }

In the above registry should be cached in memory to reduce number of SLOADs.

SLOAD 1: line 246 and the SLOAD 2: line 267

Buyout.sol.redeem() : registry should be cached (saves ~97 gas)

File: Buyout.sol line 280-288

    function redeem(address _vault, bytes32[] calldata _burnProof) external {
        // Reverts if address is not a registered vault
        (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
        if (id == 0) revert NotVault(_vault);
        // Reverts if auction state is not inactive
        (, , State current, , , ) = this.buyoutInfo(_vault);
        State required = State.INACTIVE;
        if (current != required) revert InvalidState(required, current);


        // Initializes vault transaction
        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);

In the above registry should be cached in memory to reduce number of SLOADs.

SLOAD 1 : line 280 and the SLOAD 2 : line 288

FERC1155.sol.uri() : metadata[_uri] should be cached

File: FERC1155.sol line 297-298

    function uri(uint256 _id)
        public
        view
        override(ERC1155, IFERC1155)
        returns (string memory)
    {
        require(metadata[_id] != address(0), "NO METADATA"); @audit : SLOAD 1 metadata[_id]
        return IFERC1155(metadata[_id]).uri(_id);@audit : SLOAD 2 metadata[_id]
    }

SLOAD 1: in the require statement line 297 costing 100 gas

SLOAD 2: in the return statement line 298 again costing 100gas

We can cache metadata[_id] in memory and read the value from memory instead of from storage

Help the optimizer by saving a storage variable's reference instead of repeatedly fetching migrationInfo[_vault][_proposalId]

Declare a Storage variable and use it instead of fetching the reference in the map repeatedly.

Instead of calling migrationInfo[_vault][_proposalId] everytime save it's reference like shown below and use the reference.

		Proposal storage proposal = migrationInfo[_vault][_proposalId];

File: Migration.sol line 454-456

    function migrateFractions(address _vault, uint256 _proposalId) external {
     
        // Calculates the total ether amount of a successful proposal
        uint256 totalInEth = _calculateTotal(
            1 ether,
            lastTotalSupply,
            migrationInfo[_vault][_proposalId].totalEth,
            migrationInfo[_vault][_proposalId].totalFractions
        );
        // Calculates balance of caller based on ether contribution
        uint256 balanceContributedInEth = _calculateContribution(
            totalInEth,
            lastTotalSupply,
            userProposalEth[_proposalId][msg.sender],
            userProposalFractions[_proposalId][msg.sender]
        );

        // Gets the token and fraction ID of the new vault
        address newVault = migrationInfo[_vault][_proposalId].newVault;
       

In the above function, migrationInfo[_vault][_proposalId] is being fetched 3 times in the following lines

1: line 454 2: line 455 3: line 466

Something similar to my proposal has already been implemented on line 266

Cache the length of arrays in loops (saves ~6 gas per iteration)

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: MerkleBase.sol line 51

            for (uint256 i = 0; i < _proof.length; ++i) {

The above should be modified to

	    uint256 length = _proof.length;
            for (uint256 i = 0; i < length; ++i) {

Other instances to modify File: MerkleBase.sol line 110

            for (uint256 i; i < result.length; ++i) {

File: BaseVault.sol line 64

        for (uint256 i = 0; i < _tokens.length; ) {

File: BaseVault.sol line 83

        for (uint256 i = 0; i < _tokens.length; ) {

File: BaseVault.sol line 107

            for (uint256 i = 0; i < _tokens.length; ++i) {

File: BaseVault.sol line 130

            for (uint256 i; i < _modules.length; ++i) {

File: BaseVault.sol line 132

                for (uint256 j; j < leaves.length; ++j) {

File: Buyout.sol line 454

        for (uint256 i; i < permissions.length; ) {

File: MerkleBase.sol line 78

The following shows all instances where _data.length is being accessed in the function getProof()

        require(_data.length > 1, "wont generate proof for single leaf");

        uint256 size = log2ceil_naive(_data.length);

        while (_data.length > 1) {

       } else if (_node + 1 == _data.length) {

++i costs less gas compared to i++ or i += 1 (~5 gas per iteration)

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include: File: Vault.sol line 78

        for (uint256 i = 0; i < length; i++) {
            methods[_selectors[i]] = _plugins[i];
        }

The above should be modified to:

        for (uint256 i = 0; i < length; {
            methods[_selectors[i]] = _plugins[i];
		unchecked{
			++i;
			}
        }

File: Vault.sol line 104

        for (uint256 i = 0; i < length; i++) {

use shorter revert strings(less than 32 bytes)

Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

File: MerkleBase.sol line 62

        require(_data.length > 1, "wont generate root for single leaf");

Other instances to modify File MerkleBase.sol line 78

        require(_data.length > 1, "wont generate proof for single leaf");

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

see Source

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances to modify

File: FERC1155.sol line 263-268

        require(
            msg.sender == _from ||
                isApprovedForAll[_from][msg.sender] ||
                isApproved[_from][msg.sender][_id],
            "NOT_AUTHORIZED"
        );

File: FERC1155.sol line 275

        require( _to.code.length == 0 ? _to != address(0) : INFTReceiver(_to).onERC1155Received( msg.sender, _from, _id, _amount, _data ) == INFTReceiver.onERC1155Received.selector, "UNSAFE_RECIPIENT" );
		

File: FERC1155.sol line 297

        require(metadata[_id] != address(0), "NO METADATA");

Use Shift Right/Left instead of Division/Multiplication

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

relevant source

File: MerkleBase.sol line 100

                _node = _node / 2;

The above should be modified to

		_node = _node >> 1
		

File: MerkleBase.sol line 142

                result = new bytes32[](length / 2);

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

consequences:

• Each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)

• Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

See: ethereum/solidity#9232

File: Permit.sol line 5

bytes32 constant DOMAIN_TYPEHASH = keccak256(
    "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
);

File: Permit.sol line 10

bytes32 constant PERMIT_TYPEHASH = keccak256(
    "Permit(address owner,address operator,uint256 tokenId,bool approved,uint256 nonce,uint256 deadline)"
);

File: Permit.sol line 15

bytes32 constant PERMIT_ALL_TYPEHASH = keccak256(
    "PermitAll(address owner,address operator,bool approved,uint256 nonce,uint256 deadline)"
);

constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name. Additionally, for complex values, inline comments explaining how they were calculated or why they were chosen are highly recommended. Following Solidity’s style guide, constants should be named in UPPER_CASE_WITH_UNDERSCORES format, and specific public getters should be defined to read each one of them.

File: Buyout.sol line 209

            (tokenBalance * 1000) /

File: Buyout.sol line 211

            500

File: Buyout.sol line 86-87

        uint256 buyoutPrice = (msg.value * 100) /
            (100 - ((depositAmount * 100) / totalSupply));

File: FERC1155.sol line 247

        royaltyAmount = (_salePrice * royaltyPercent[_id]) / 100;

Using Private Rather than Public for constants saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non payable getter functions for deployment calldata, and not adding another entry to the method ID table

File: FERC1155.sol line 15

    string public constant NAME = "FERC1155";

File: FERC1155.sol line 17

    string public constant VERSION = "1";

File: Buyout.sol line 35

    uint256 public constant PROPOSAL_PERIOD = 2 days;

File: Buyout.sol line 37

    uint256 public constant REJECTION_PERIOD = 4 days;

File: Migration.sol line 43

    uint256 public constant PROPOSAL_PERIOD = 7 days;

Use CALLDATA Instead of Memory

File: FERC1155.sol line 68-72

When arguments are read only on external functions, the data location should be calldata avoiding the cost of allocating memory or storage.

    function emitSetURI(uint256 _id, string memory _uri) external {
        if (msg.sender != metadata[_id])
            revert InvalidSender(metadata[_id], msg.sender);
        emit URI(_uri, _id);
    }

string memory _uri should be modified to string calldata _uri

File: FERC1155.sol line 79-87

    function mint(
        address _to,
        uint256 _id,
        uint256 _amount,
        bytes memory _data
    ) external onlyRegistry {
        _mint(_to, _id, _amount, _data);
        totalSupply[_id] += _amount;
    }

Since _data is only being read and not modified here, we can modify our bytes memory _data to bytes calldata _data

File: Metadata.sol line 24-31

    function setURI(uint256 _id, string memory _uri) external {
        address controller = IFERC1155(token).controller();
        if (msg.sender != controller)
            revert IFERC1155.InvalidSender(controller, msg.sender);


        tokenMetadata[_id] = _uri;
        IFERC1155(token).emitSetURI(_id, _uri);
    }

Since _uri is only being read we can use calldata instead of memory : from string memory _uri to string calldata _uri