Fractional v2 contest - TomJ'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

Impact

It is recommended to avoid using payable.transfer, since it can cause the transaction to fail when the user is accessing this function with a smart contract and:

• does not have payable function
• have a payable function but spends more than 2300 gas
• have a payable function and spends less than 2300 gas but is called through proxy which uses over 2300 gas.

Reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Proof of Concept

./modules/Migration.sol:172:        payable(msg.sender).transfer(ethAmount);
./modules/Migration.sol:325:        payable(msg.sender).transfer(userEth);

Tools Used

Manual Analysis

Recommended Mitigation Steps

I recommend using low-level call() or OpenZeppelin Address.sendValue instead of transfer().

Table of Contents

Low Risk Issues

• No Access Control in _from parameter of transferFrom
• Admin Address Change should be a 2-Step Process
• Immutable addresses should 0-Check

Non-critical Issues

• Should Resolve TODOs before Deployment
• Event is Missing Indexed Fields

 

Low Risk Issues

No Access Control in _from parameter of transferFrom

Issue

Below 3 functions shown in the PoC is external function which calls transferFrom / safeTransferFrom with unauthorized "_from" parameter. It is recommended to hard code "_from" parameter to msg.sender.

PoC

1. batchDepositERC20() of BaseVault.sol https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L65

2. batchDepositERC721() of BaseVault.sol https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L84

3. batchDepositERC1155() of BaseVault.sol https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L108-L114

Mitigation

I recommend to hard code "_from" parameter of transferFrom / safeTransferFrom to msg.sender.

 

Admin Address Change should be a 2-Step Process

Issue

High privilege account such as admin / owner is changed with only single process. This can be a concern since an admin / owner role has a high privilege in the contract and mistakenly setting a new admin to an incorrect address will end up losing that privilege.

PoC

93:    function transferOwnership(address _newOwner) external {
94:        if (owner != msg.sender) revert NotOwner(owner, msg.sender);
95:        owner = _newOwner;
96:        emit TransferOwnership(msg.sender, _newOwner);
97:    }

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L93-L97

229:    function transferController(address _newController)
230:        external
231:        onlyController
232:    {
233:        if (_newController == address(0)) revert ZeroAddress();
234:        _controller = _newController;
235:        emit ControllerTransferred(_newController);
236:    }

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L229-L236

Mitigation

This can be fixed by implementing 2-step process. We can do this by following. First make the function that sets high privilege account approve a new address as a pending admin / owner. Next that pending admin / owner has to claim the ownership in a separate transaction to be a new admin / owner.

 

Immutable addresses should 0-Check

Issue

I recommend adding check of 0-address for immutable addresses. Not doing so might lead to non-functional contract when it is updated to 0-address accidentally.

PoC

Total of 3 issues found

• Supply.sol: 1 immutable address of "registry" https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/targets/Supply.sol#L16-L18
• SupplyReference.sol: 1 immutable address of "registry" https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/references/SupplyReference.sol#L15-L17
• Metadata.sol: 1 immutable address of "token" https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/Metadata.sol#L16-L18

Mitigation

Add 0-address check for above 3 immutable addresses.

 

Non-critical Issues

Should Resolve TODOs before Deployment

Issue

Questions/Issues in the code should be resolved before the deployment.

PoC

./src/utils/MerkleBase.sol:24:            // TODO: This can be aesthetically simplified with a switch. Not sure it will

Mitigation

Resolve before deployment and delete the comment.

 

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

./src/interfaces/IERC20.sol:6-10:    event Approval(address indexed _owner, address indexed _spender, uint256 _amount);
./src/interfaces/IERC20.sol:11:    event Transfer(address indexed _from, address indexed _to, uint256 amount);
./src/interfaces/IERC1155.sol:6:    event ApprovalForAll(address indexed _owner, address indexed _operator, bool _approved);
./src/interfaces/IERC1155.sol:25:    event URI(string _value, uint256 indexed _id);
./src/interfaces/IVault.sol:25:    event Execute(address indexed _target, bytes _data, bytes _response);
./src/interfaces/IVault.sol:29:    event InstallPlugin(bytes4[] _selectors, address[] _plugins);
./src/interfaces/IVault.sol:39:    event UninstallPlugin(bytes4[] _selectors);
./src/interfaces/IBuyout.sol:55:    event Start(address indexed _vault, address indexed _proposer, uint256 _startTime, uint256 _buyoutPrice, uint256 _fractionPrice);
./src/interfaces/IBuyout.sol:65:    event SellFractions(address indexed _seller, uint256 _amount);
./src/interfaces/IBuyout.sol:69:    event BuyFractions(address indexed _buyer, uint256 _amount);
./src/interfaces/IBuyout.sol:74:    event End(address _vault, State _state, address indexed _proposer);
./src/interfaces/IBuyout.sol:79:    event Cash(address _vault, address indexed _casher, uint256 _amount);
./src/interfaces/IBuyout.sol:83:    event Redeem(address _vault, address indexed _redeemer);
./src/interfaces/IERC721.sol:11:    event ApprovalForAll(address indexed _owner, address indexed _operator, bool _approved);
./src/interfaces/IVaultRegistry.sol:33:    event VaultDeployed(address indexed _vault, address indexed _token, uint256 _id);
./src/interfaces/IBaseVault.sol:12:    event ActiveModules(address indexed _vault, address[] _modules);
./src/interfaces/IMigration.sol:61:    event FractionsMigrated(address indexed _oldVault, address indexed _newVault, uint256 _proposalId, uint256 _amount);
./src/interfaces/IMigration.sol:74:    event VaultMigrated(address indexed _oldVault, address indexed _newVault, uint256 _proposalId, address[] _modules, address[] _plugins, bytes4[] _selectors);
./src/interfaces/IFERC1155.sol:21:    event SetMetadata(address indexed _metadata, uint256 _id);
./src/interfaces/IFERC1155.sol:26:    event SetRoyalty(address indexed _receiver, uint256 _id, uint256 _percentage);
./src/interfaces/IFERC1155.sol:36:    event SingleApproval(address indexed _owner, address indexed _operator, uint256 _id, bool _approved);

Mitigation

Add up to 3 indexed fields when possible.

 

Table of Contents

• Minimize the Number of SLOADs by Caching State Variable
• Defined Variables Used Only Once
• Internal Function Called Only Once Should be Inlined
• Duplicate require() Checks Should be a Modifier or a Function
• Use Calldata instead of Memory for Read Only Function Parameters
• Constant Value of a Call to keccak256() should Use Immutable
• Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas
• Unnecessary Default Value Initialization
• Store Array's Length as a Variable
• ++i Costs Less Gas than i++
• Keep The Revert Strings of Error Messages within 32 Bytes
• Use Custom Errors to Save Gas

 

Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC

1. FERC1155.sol

• Cache _controller of function controller 2 SLOADs to 1 SLOAD, 1 MSTORE and 1 MLOAD https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L303 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L305

• Cache metadata of function emitSetURI 2 SLOADs to 1 SLOAD, 1 MSTORE and 1 MLOAD https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L69-L70

• Cache metadata of function uri 2 SLOADs to 1 SLOAD, 1 MSTORE and 1 MLOAD https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L297-L298

2. VaultFactory.sol

• Cache nextSeeds of function deployFor 2 SLOADs to 1 SLOAD, 1 MSTORE and 1 MLOAD https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/VaultFactory.sol#L63 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/VaultFactory.sol#L77

 

Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

 

Defined Variables Used Only Once

Issue

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

PoC

1. Vault.sol

• Remove 'plugin' variable of fallback function

39:        address plugin = methods[msg.sig];
40:        (,response) = _execute(plugin, _data);

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L39-L40

Mitigation: Delete line 39 and replace line 40 with below line

        (,response) = _execute(methods[msg.sig], _data);

Mitigation

Don't define variable that is used only once. Details are listed on above PoC.

 

Internal Function Called Only Once Should be Inlined

Issue

Certain function is defined even though it is called only once. Inline it instead to where it is called to avoid usage of extra gas.

PoC

1. _revertedWithReason() of Vault.sol _revertedWithReason function called only once at line 137 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L142-L146 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L137

2. _revertedWithReason() of Multicall.sol _revertedWithReason function called only once at line 25 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/Multicall.sol#L39-L43 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/Multicall.sol#L25

3. _computePermitStructHash() of FERC1155.sol _computePermitStructHash function called only once at line 113 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L324-L342 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L113

4. _computePermitAllStructHash() of FERC1155.sol _computePermitAllStructHash function called only once at line 159 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L350-L366 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L159-L164

5. _attemptETHTransfer() of SafeSend.sol _attemptETHTransfer function called only once at line 31 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/SafeSend.sol#L18-L25 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/SafeSend.sol#L31

Mitigation

I recommend to not define above functions and instead inline it to place it is called.

 

Duplicate require / revert Checks Should be a Modifier or a Function

Issue

Below require / revert checks are used more than once, I recommend making these to a modifier or a function to save gas.

PoC

Total of 11 of these issue was found.

./src/FERC1155.sol:108-109:            if (block.timestamp > _deadline) revert SignatureExpired(block.timestamp, _deadline);
./src/FERC1155.sol:154-155:            if (block.timestamp > _deadline) revert SignatureExpired(block.timestamp, _deadline);

./src/FERC1155.sol:128-129:            if (signer == address(0) || signer != _owner) revert InvalidSignature(signer, _owner);
./src/FERC1155.sol:173-174:            if (signer == address(0) || signer != _owner) revert InvalidSignature(signer, _owner);

./src/Vault.sol:76:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);
./src/Vault.sol:87:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);
./src/Vault.sol:94:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);
./src/Vault.sol:102:        if (owner != msg.sender) revert NotOwner(owner, msg.sender);

./src/modules/Buyout.sol:64:         if (id == 0) revert NotVault(_vault);
./src/modules/Buyout.sol:117:        if (id == 0) revert NotVault(_vault);
./src/modules/Buyout.sol:154:        if (id == 0) revert NotVault(_vault);
./src/modules/Buyout.sol:189:        if (id == 0) revert NotVault(_vault);
./src/modules/Buyout.sol:249:        if (id == 0) revert NotVault(_vault);
./src/modules/Buyout.sol:281:        if (id == 0) revert NotVault(_vault);
./src/modules/Buyout.sol:320:        if (id == 0) revert NotVault(_vault);
./src/modules/Buyout.sol:352:        if (id == 0) revert NotVault(_vault);
./src/modules/Buyout.sol:388:        if (id == 0) revert NotVault(_vault);
./src/modules/Buyout.sol:425:        if (id == 0) revert NotVault(_vault);

./src/modules/Buyout.sol:68:         if (current != required) revert InvalidState(required, current);
./src/modules/Buyout.sol:122:        if (current != required) revert InvalidState(required, current);
./src/modules/Buyout.sol:159:        if (current != required) revert InvalidState(required, current);
./src/modules/Buyout.sol:200:        if (current != required) revert InvalidState(required, current);
./src/modules/Buyout.sol:253:        if (current != required) revert InvalidState(required, current);
./src/modules/Buyout.sol:285:        if (current != required) revert InvalidState(required, current);
./src/modules/Buyout.sol:324:        if (current != required) revert InvalidState(required, current);
./src/modules/Buyout.sol:356:        if (current != required) revert InvalidState(required, current);
./src/modules/Buyout.sol:392:        if (current != required) revert InvalidState(required, current);
./src/modules/Buyout.sol:429:        if (current != required) revert InvalidState(required, current);

./src/modules/Buyout.sol:126:            revert TimeExpired(block.timestamp, endTime);
./src/modules/Buyout.sol:163:            revert TimeExpired(block.timestamp, endTime);

./src/modules/Buyout.sol:326:        if (msg.sender != proposer) revert NotWinner();
./src/modules/Buyout.sol:358:        if (msg.sender != proposer) revert NotWinner();
./src/modules/Buyout.sol:394:        if (msg.sender != proposer) revert NotWinner();
./src/modules/Buyout.sol:431:        if (msg.sender != proposer) revert NotWinner();

./src/modules/Migration.sol:82:         if (id == 0) revert NotVault(_vault);
./src/modules/Migration.sol:114:        if (id == 0) revert NotVault(_vault);
./src/modules/Migration.sol:146:        if (id == 0) revert NotVault(_vault);
./src/modules/Migration.sol:187:        if (id == 0) revert NotVault(_vault);
./src/modules/Migration.sol:299:        if (id == 0) revert NotVault(_vault);
./src/modules/Migration.sol:436:        if (id == 0) revert NotVault(_vault);

./src/modules/Migration.sol:86:         if (current != required) revert IBuyout.InvalidState(required, current);
./src/modules/Migration.sol:118:        if (current != required) revert IBuyout.InvalidState(required, current);
./src/modules/Migration.sol:150:        if (current != required) revert IBuyout.InvalidState(required, current);
./src/modules/Migration.sol:191:        if (current != required) revert IBuyout.InvalidState(required, current);
./src/modules/Migration.sol:442:        if (current != required) revert IBuyout.InvalidState(required, current);

./src/modules/Migration.sol:226:        if (current != State.SUCCESS) revert UnsuccessfulMigration();
./src/modules/Migration.sol:264:        if (current != State.SUCCESS) revert UnsuccessfulMigration();

./src/VaultRegistry.sol:42:         if (id == 0) revert UnregisteredVault(msg.sender);
./src/VaultRegistry.sol:120:        if (id == 0) revert UnregisteredVault(msg.sender);

Mitigation

I recommend making duplicate require statement into modifier or a function.

 

Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

./src/FERC1155.sol:68:    function emitSetURI(uint256 _id, string memory _uri) external {
./src/FERC1155.sol:83:        bytes memory _data
./src/FERC1155.sol:261:        bytes memory _data
./src/utils/Metadata.sol:24:    function setURI(uint256 _id, string memory _uri) external {
./src/utils/Multicall.sol:39:    function _revertedWithReason(bytes memory _response) internal pure {
./src/utils/MerkleBase.sol:45:        bytes32[] memory _proof,
./src/utils/MerkleBase.sol:61:    function getRoot(bytes32[] memory _data) public pure returns (bytes32) {
./src/utils/MerkleBase.sol:73:    function getProof(bytes32[] memory _data, uint256 _node)
./src/utils/MerkleBase.sol:125:    function hashLevel(bytes32[] memory _data)
./src/Vault.sol:73:    function install(bytes4[] memory _selectors, address[] memory _plugins)
./src/Vault.sol:101:    function uninstall(bytes4[] memory _selectors) external {
./src/Vault.sol:142:    function _revertedWithReason(bytes memory _response) internal pure {
./src/modules/Migration.sol:487:    function generateMerkleTree(address[] memory _modules)
./src/VaultRegistry.sol:53:        address[] memory _plugins,
./src/VaultRegistry.sol:54:        bytes4[] memory _selectors
./src/VaultRegistry.sol:70:        address[] memory _plugins,
./src/VaultRegistry.sol:71:        bytes4[] memory _selectors
./src/VaultRegistry.sol:85:        address[] memory _plugins,
./src/VaultRegistry.sol:86:        bytes4[] memory _selectors
./src/VaultRegistry.sol:105:        address[] memory _plugins,
./src/VaultRegistry.sol:106:        bytes4[] memory _selectors
./src/VaultRegistry.sol:150:        address[] memory _plugins,
./src/VaultRegistry.sol:151:        bytes4[] memory _selectors
./src/VaultRegistry.sol:168:        address[] memory _plugins,
./src/VaultRegistry.sol:169:        bytes4[] memory _selectors
./src/interfaces/IERC1155.sol:29:    function balanceOfBatch(address[] memory _owners, uint256[] memory ids)
./src/interfaces/IERC1155.sol:39:        uint256[] memory _ids,
./src/interfaces/IERC1155.sol:40:        uint256[] memory _amounts,
./src/interfaces/IERC1155.sol:41:        bytes memory _data
./src/interfaces/IERC1155.sol:49:        bytes memory _data
./src/interfaces/IVault.sol:43:        bytes memory _data,
./src/interfaces/IVault.sol:44:        bytes32[] memory _proof
./src/interfaces/IVault.sol:49:    function install(bytes4[] memory _selectors, address[] memory _plugins)
./src/interfaces/IBuyout.sol:93:        uint256[] memory _ids,
./src/interfaces/IBuyout.sol:94:        uint256[] memory _values,
./src/interfaces/IBuyout.sol:95:        bytes32[] memory _erc1155BatchTransferProof
./src/interfaces/IBuyout.sol:140:        bytes32[] memory _erc20TransferProof
./src/interfaces/IBuyout.sol:148:        bytes32[] memory _erc721TransferProof
./src/interfaces/IBuyout.sol:157:        bytes32[] memory _erc1155TransferProof
./src/interfaces/IERC721.sol:44:        bytes memory _data
./src/interfaces/IVaultRegistry.sol:43:        address[] memory _plugins,
./src/interfaces/IVaultRegistry.sol:44:        bytes4[] memory _selectors
./src/interfaces/IVaultRegistry.sol:49:        address[] memory _plugins,
./src/interfaces/IVaultRegistry.sol:50:        bytes4[] memory _selectors
./src/interfaces/IVaultRegistry.sol:56:        address[] memory _plugins,
./src/interfaces/IVaultRegistry.sol:57:        bytes4[] memory _selectors
./src/interfaces/IVaultRegistry.sol:63:        address[] memory _plugins,
./src/interfaces/IVaultRegistry.sol:64:        bytes4[] memory _selectors
./src/interfaces/IVaultRegistry.sol:70:        address[] memory _plugins,
./src/interfaces/IVaultRegistry.sol:71:        bytes4[] memory _selectors
./src/interfaces/IBaseVault.sol:17:        address[] memory _tokens,
./src/interfaces/IBaseVault.sol:18:        uint256[] memory _amounts
./src/interfaces/IBaseVault.sol:24:        address[] memory _tokens,
./src/interfaces/IBaseVault.sol:25:        uint256[] memory _ids
./src/interfaces/IBaseVault.sol:31:        address[] memory _tokens,
./src/interfaces/IBaseVault.sol:32:        uint256[] memory _ids,
./src/interfaces/IBaseVault.sol:33:        uint256[] memory _amounts,
./src/interfaces/IBaseVault.sol:34:        bytes[] memory _datas
./src/interfaces/IBaseVault.sol:39:        address[] memory _modules,
./src/interfaces/IBaseVault.sol:42:        bytes32[] memory _mintProof
./src/interfaces/IMigration.sol:89:        uint256[] memory _ids,
./src/interfaces/IMigration.sol:90:        uint256[] memory _amounts,
./src/interfaces/IMigration.sol:91:        bytes32[] memory _erc1155BatchTransferProof
./src/interfaces/IMigration.sol:100:    function generateMerkleTree(address[] memory _modules)
./src/interfaces/IMigration.sol:120:        bytes32[] memory _erc20TransferProof
./src/interfaces/IMigration.sol:128:        bytes32[] memory _erc721TransferProof
./src/interfaces/IMigration.sol:150:        address[] memory _modules,
./src/interfaces/IMigration.sol:151:        address[] memory _plugins,
./src/interfaces/IMigration.sol:152:        bytes4[] memory _selectors,
./src/interfaces/IMigration.sol:162:        bytes32[] memory _mintProof
./src/interfaces/IFERC1155.sol:75:        bytes memory _data
./src/interfaces/IFERC1155.sol:111:        bytes memory _data

Mitigation

Change memory to calldata

 

Constant Value of a Call to keccak256() should Use Immutable

Issue

When using constant it is expected that the value should be converted into a constant value at compile time. However when using a call to keccak256(), the expression is re-calculated each time the constant is referenced. Resulting in costing about 100 gas more on each access to this constant. link for more details: https://github.com/ethereum/solidity/issues/9232

PoC

./src/constants/Permit.sol:5-7:bytes32 constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
./src/constants/Permit.sol:10-12:bytes32 constant PERMIT_TYPEHASH = keccak256("Permit(address owner,address operator,uint256 tokenId,bool approved,uint256 nonce,uint256 deadline)");
./src/constants/Permit.sol:15-17:bytes32 constant PERMIT_ALL_TYPEHASH = keccak256("PermitAll(address owner,address operator,bool approved,uint256 nonce,uint256 deadline)");

Mitigation

Change the variable from constant to immutable.

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, it acctually cost more gas to use elements smaller than 32 bytes. Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

./src/FERC1155.sol:104:        uint8 _v,
./src/FERC1155.sol:150:        uint8 _v,
./src/utils/SelfPermit.sol:23:        uint8 _v,
./src/utils/SelfPermit.sol:50:        uint8 _v,
./src/interfaces/IERC20.sol:21:    function decimals() external view returns (uint8);
./src/interfaces/IERC20.sol:32:        uint8 _v,
./src/interfaces/IFERC1155.sol:86:        uint8 _v,
./src/interfaces/IFERC1155.sol:96:        uint8 _v,

Mitigation

I suggest using uint256 instead of anything smaller.

 

Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

./src/utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {
./src/Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
./src/Vault.sol:104:        for (uint256 i = 0; i < length; i++) {
./src/modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) {
./src/modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) {
./src/modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {

Mitigation

I suggest removing default value initialization. For example,

• for (uint256 i; i < proof.length; ++i) {

 

Store Array's Length as a Variable

Issue

By storing an array's length as a variable before the for-loop, can save 3 gas per iteration.

PoC

./src/utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {
./src/utils/MerkleBase.sol:110:            for (uint256 i; i < result.length; ++i) {
./src/modules/Buyout.sol:454:        for (uint256 i; i < permissions.length; ) {
./src/modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) {
./src/modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) {
./src/modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
./src/modules/protoforms/BaseVault.sol:130:            for (uint256 i; i < _modules.length; ++i) {
./src/modules/protoforms/BaseVault.sol:132:                for (uint256 j; j < leaves.length; ++j) {

Mitigation

Store array's length as a variable before looping it. For example, I suggest changing it to

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

 

++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

./src/FERC1155.sol:339:                    nonces[_owner]++,
./src/FERC1155.sol:363:                    nonces[_owner]++,
./src/utils/MerkleBase.sol:188:                ceil++;
./src/Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
./src/Vault.sol:104:        for (uint256 i = 0; i < length; i++) {
./src/modules/protoforms/BaseVault.sol:133:                    hashes[counter++] = leaves[j];
./src/modules/Migration.sol:508:                    hashes[counter++] = leaves[j];

Mitigation

Change it to postfix increments/decrements. For example,

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

 

Keep The Revert Strings of Error Messages within 32 Bytes

Issue

Since each storage slot is size of 32 bytes, error messages that is longer than this will need extra storage slot leading to more gas cost.

PoC

./src/utils/MerkleBase.sol:62:        require(_data.length > 1, "wont generate root for single leaf");
./src/utils/MerkleBase.sol:78:        require(_data.length > 1, "wont generate proof for single leaf");

Mitigation

Simply keep the error messages within 32 bytes to avoid extra storage slot cost.

 

Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

./src/FERC1155.sol:263-268:        require(msg.sender == _from || isApprovedForAll[_from][msg.sender] || isApproved[_from][msg.sender][_id], "NOT_AUTHORIZED");
./src/FERC1155.sol:275-286:        require(_to.code.length == 0 ? _to != address(0) : INFTReceiver(_to).onERC1155Received(msg.sender, _from, _id, _amount, _data) == INFTReceiver.onERC1155Received.selector, "UNSAFE_RECIPIENT");
./src/FERC1155.sol:297:        require(metadata[_id] != address(0), "NO METADATA");
./src/utils/MerkleBase.sol:62:        require(_data.length > 1, "wont generate root for single leaf");
./src/utils/MerkleBase.sol:78:        require(_data.length > 1, "wont generate proof for single leaf");

Mitigation

I suggest implementing custom errors to save gas.