Fractional v2 contest - BowTiedWardens's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L292

Vulnerability details

Issue/consequence: users can withdraw their contributions from migrations that are not committed yet causing a malfunction of the protocol. E.g. someone can add proposals then it can withdraw them using this method, as the totalEth and totalFractions of a proposal are not decreased during this method, the users can manipulate the price on commit. This is possible due to the fact that there is no correlation between _vault parameter and _proposalId, so someone can call this function with a vault that is not migrated yet and different proposalIds.

Affected Code

File: Migration.sol
289:     /// @notice Retrieves ether and fractions deposited from an unsuccessful migration
290:     /// @param _vault Address of the vault
291:     /// @param _proposalId ID of the failed proposal
292:     function withdrawContribution(address _vault, uint256 _proposalId)                 // @audit-info [HIGH]  
293:         external
294:     {
295:         // Reverts if address is not a registered vault
296:         (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
297:             _vault
298:         );
299:         if (id == 0) revert NotVault(_vault);
300:         // Reverts if caller has no fractional balance to withdraw
301:         (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
302:         if (
303:             current != State.INACTIVE ||
304:             migrationInfo[_vault][_proposalId].newVault != address(0)
305:         ) revert NoContributionToWithdraw();
306: 
307:         // Temporarily store user's fractions for the transfer
308:         uint256 userFractions = userProposalFractions[_proposalId][msg.sender];
309:         // Updates fractional balance of caller
310:         userProposalFractions[_proposalId][msg.sender] = 0;
311:         // Withdraws fractional tokens from contract back to caller
312:         IFERC1155(token).safeTransferFrom(
313:             address(this),
314:             msg.sender,
315:             id,
316:             userFractions,
317:             ""
318:         );
319: 
320:         // Temporarily store user's eth for the transfer
321:         uint256 userEth = userProposalEth[_proposalId][msg.sender];
322:         // Updates ether balance of caller
323:         userProposalEth[_proposalId][msg.sender] = 0;
324:         // Withdraws ether from contract back to caller
325:         payable(msg.sender).transfer(userEth);
326:     }

Mitigation

Recommendation: subtract totalFractions and totalEth when withdrawing (like you do on the leave function) or make sure the migrateInfo and proposalId are correlated. Currently there is no check between proposalId and vault, except newVault which is address(0) most of the time.

File: Migration.sol
302:         if (
303:             current != State.INACTIVE ||
304:             migrationInfo[_vault][_proposalId].newVault != address(0)
305:         ) revert NoContributionToWithdraw();

Lines of code

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

Vulnerability details

This is a classic Code4rena issue:

• https://github.com/code-423n4/2021-04-meebits-findings/issues/2
• https://github.com/code-423n4/2021-10-tally-findings/issues/20
• https://github.com/code-423n4/2022-01-openleverage-findings/issues/75

Impact

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

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

Impacted lines

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

Recommended Mitigation

Use call() instead of transfer(), but be sure to implement CEI patterns in CEther and add a global state lock on the comptroller as per Rari.

THIS HAS REKT COMPOUND FORKS BEFORE!!!

relevant links: https://twitter.com/hacxyk/status/1520715516490379264?s=21&t=fnhDkcC3KpE_kJE8eLiE2A https://twitter.com/hacxyk/status/1520715536325218304?s=21&t=fnhDkcC3KpE_kJE8eLiE2A https://twitter.com/hacxyk/status/1520370441705037824?s=21&t=fnhDkcC3KpE_kJE8eLiE2A https://twitter.com/Hacxyk/status/1521949933380595712

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. Places where the nonReentrant modifier should be applied
  • 1.2. Places where the Check Effects Interactions Pattern should be respected
  • 1.3. Missing address(0) checks
  • 1.4. Unsafe use of transfer()/transferFrom() with IERC20
  • 1.5. Unused/empty receive() function
  • 1.6. Prevent accidentally burning tokens
  • 1.7. _safeMint() should be used rather than _mint() wherever possible
  • 1.8. Use a 2-step ownership transfer pattern
• 2. Non-Critical Issues
  • 2.1. It's better to emit after all processing is done
  • 2.2. Typos
  • 2.3. Open TODOS
  • 2.4. Use string.concat() or bytes.concat()
  • 2.5. public functions not called by the contract should be declared external instead

1. Low Risk Issues

1.1. Places where the nonReentrant modifier should be applied

external/public functions without access-control that modify the state after an external call should be nonReentrant as a best practice to reduce the attack surface.

Keep in mind that IERC1155(token).safeTransferFrom() has a callback on onERC1155Received that opens way for a potential re-entrancy:

File: ERC1155.sol
55:     function safeTransferFrom(
...
69:         require(
70:             to.code.length == 0
71:                 ? to != address(0)
72:                 : ERC1155TokenReceiver(to).onERC1155Received(msg.sender, from, id, amount, data) ==
73:                     ERC1155TokenReceiver.onERC1155Received.selector,
74:             "UNSAFE_RECIPIENT"
75:         );
76:     }

Here, the Check Effects Interactions Pattern cannot be applied (the values used are determined after the external call). The nonReentrant modifier should then be used here (see @audit tag):

File: Buyout.sol
57:     function start(address _vault) external payable { //@audit should be nonReentrant
...
75:         // Transfers fractional tokens into the buyout pool
76:         IERC1155(token).safeTransferFrom( //@audit : external call (Interaction)
77:             msg.sender,
78:             address(this),
79:             id,
80:             depositAmount,
81:             ""
82:         );
...
91:         buyoutInfo[_vault] = Auction( //@audit : modifying the state after the external call (Effect, CEIP not respected). Consider marking this function as nonReentrant
92:             block.timestamp,
93:             msg.sender,
94:             State.LIVE,
95:             fractionPrice,
96:             msg.value,
97:             totalSupply
98:         );

1.2. Places where the Check Effects Interactions Pattern should be respected

Consider moving the state-changes before the external calls here:

• sellFractions

File: Buyout.sol
112:     function sellFractions(address _vault, uint256 _amount) external {
...
128:         // Transfers fractional tokens to pool from caller
129:         IERC1155(token).safeTransferFrom(  //@audit CEIP not respected + 
130:             msg.sender,
131:             address(this),
132:             id,
133:             _amount,
134:             ""
135:         );
136: 
137:         // Updates ether balance of pool
138:         uint256 ethAmount = fractionPrice * _amount;
139:         buyoutInfo[_vault].ethBalance -= ethAmount; //@audit CEIP not respected
140:         // Transfers ether amount to caller
141:         _sendEthOrWeth(msg.sender, ethAmount);
142:         // Emits event for selling fractions into pool
143:         emit SellFractions(msg.sender, _amount);
144:     }

• buyFractions

File: Buyout.sol
149:     function buyFractions(address _vault, uint256 _amount) external payable {
...
167:         // Transfers fractional tokens to caller
168:         IERC1155(token).safeTransferFrom(
169:             address(this),
170:             msg.sender,
171:             id,
172:             _amount,
173:             ""
174:         );
175:         // Updates ether balance of pool
176:         buyoutInfo[_vault].ethBalance += msg.value;
177:         // Emits event for buying fractions from pool
178:         emit BuyFractions(msg.sender, _amount);

• end

File: Buyout.sol
184:     function end(address _vault, bytes32[] calldata _burnProof) external {
...
228:             IERC1155(token).safeTransferFrom(//@audit CEIP not respected
229:                 address(this),
230:                 proposer,
231:                 id,
232:                 tokenBalance,
233:                 ""
234:             );
235:             _sendEthOrWeth(proposer, ethBalance);//@audit CEIP not respected

• join

File: Migration.sol
105:     function join(
....
126:         IFERC1155(token).safeTransferFrom( //@audit CEIP not respected
127:             msg.sender,
128:             address(this),
129:             id,
130:             _amount,
131:             ""
132:         );
133:         // Updates fraction balances of the proposal and caller
134:         proposal.totalFractions += _amount; //@audit CEIP not respected
135:         userProposalFractions[_proposalId][msg.sender] += _amount; //@audit CEIP not respected

• withdrawContribution

File: Migration.sol
292:     function withdrawContribution(address _vault, uint256 _proposalId)
...
312:         IFERC1155(token).safeTransferFrom( //@audit CEIP not respected
313:             address(this),
314:             msg.sender,
315:             id,
316:             userFractions,
317:             ""
318:         );
319: 
320:         // Temporarily store user's eth for the transfer
321:         uint256 userEth = userProposalEth[_proposalId][msg.sender];
322:         // Udpates ether balance of caller
323:         userProposalEth[_proposalId][msg.sender] = 0; //@audit CEIP not respected
324:         // Withdraws ether from contract back to caller
325:         payable(msg.sender).transfer(userEth);

1.3. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

• registry

File: SupplyReference.sol
12:     address immutable registry;
13: 
14:     /// @notice Initializes address of registry contract
15:     constructor(address _registry) {
16:         registry = _registry;
17:     }

• registry

File: Supply.sol
12:     /// @notice Address of VaultRegistry contract
13:     address immutable registry;
14: 
15:     /// @notice Initializes registry contract
16:     constructor(address _registry) {
17:         registry = _registry;
18:     }

• token

File: Metadata.sol
11:     address immutable token;
...
15:     /// @notice Initializes token contract
16:     constructor(address _token) {
17:         token = _token;
18:     }

• factory, fNFT and fNFTImplementation

File: VaultRegistry.sol
17:     address public immutable factory;
18:     /// @notice Address of FERC1155 token contract
19:     address public immutable fNFT;
20:     /// @notice Address of Implementation for FERC1155 token contract
21:     address public immutable fNFTImplementation;
...
27:     /// @notice Initializes factory, implementation, and token contracts
28:     constructor() {
29:         factory = address(new VaultFactory());
30:         fNFTImplementation = address(new FERC1155());
31:         fNFT = fNFTImplementation.clone(
32:             abi.encodePacked(msg.sender, address(this))
33:         );
34:     }

1.4. Unsafe use of transfer()/transferFrom() with IERC20

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead or check the return value.

Bad:

IERC20(token).transferFrom(msg.sender, address(this), amount);

Good (using OpenZeppelin's SafeERC20):

import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";

// ...

IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

Good (using require):

bool success = IERC20(token).transferFrom(msg.sender, address(this), amount);
require(success, "ERC20 transfer failed");

Consider using safeTransfer/safeTransferFrom or wrap in a require statement here:

• TransferReference.sol#ERC20Transfer()

File: TransferReference.sol
17:     function ERC20Transfer(
18:         address _token,
19:         address _to,
20:         uint256 _value
21:     ) external {
22:         IERC20(_token).transfer(_to, _value);
23:     }

• BaseVault.sol#batchDepositERC20()

File: BaseVault.sol
58:     function batchDepositERC20(
59:         address _from,
60:         address _to,
61:         address[] calldata _tokens,
62:         uint256[] calldata _amounts
63:     ) external {
64:         for (uint256 i = 0; i < _tokens.length; ) {
65:             IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);
66:             unchecked {
67:                 ++i;
68:             }
69:         }
70:     }

1.5. Unused/empty receive() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

modules/Buyout.sol:53:    receive() external payable {}
modules/Migration.sol:63:    receive() external payable {}
Vault.sol:32:    receive() external payable {}

1.6. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

• TransferReference.sol#ERC20Transfer()

File: TransferReference.sol
17:     function ERC20Transfer(
18:         address _token,
19:         address _to,
20:         uint256 _value
21:     ) external {
22:         IERC20(_token).transfer(_to, _value);
23:     }

• BaseVault.sol#batchDepositERC20()

File: BaseVault.sol
58:     function batchDepositERC20(
59:         address _from,
60:         address _to,
61:         address[] calldata _tokens,
62:         uint256[] calldata _amounts
63:     ) external {
64:         for (uint256 i = 0; i < _tokens.length; ) {
65:             IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);
66:             unchecked {
67:                 ++i;
68:             }
69:         }
70:     }

1.7. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements ERC1155TokenReceiver's onERC1155Received.

File: FERC1155.sol
79:     function mint(
80:         address _to,
81:         uint256 _id,
82:         uint256 _amount,
83:         bytes memory _data
84:     ) external onlyRegistry {
85:         _mint(_to, _id, _amount, _data); //@audit use safeMint
86:         totalSupply[_id] += _amount;
87:     }

Be careful however to respect the CEI pattern or add a re-entrancy guard as _safeMint adds a callback-check and a malicious onERC1155Received could be exploited if not careful.

Here, totalSupply[_id] += _amount; will need to be moved before the new _safeMint.

Reading material:

• https://blocksecteam.medium.com/when-safemint-becomes-unsafe-lessons-from-the-hypebears-security-incident-2965209bda2a
• https://samczsun.com/the-dangers-of-surprising-code/
• https://github.com/KadenZipfel/smart-contract-attack-vectors/blob/master/vulnerabilities/unprotected-callback.md

1.8. Use a 2-step ownership transfer pattern

It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

File: Vault.sol
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:     }

2. Non-Critical Issues

2.1. It's better to emit after all processing is done

src/modules/protoforms/BaseVault.sol:
  48:         emit ActiveModules(vault, _modules);
  49  
  50          _mintFractions(vault, msg.sender, _fractionSupply, _mintProof);
  51      }

2.2. Typos

interfaces/IMigration.sol:23:    // Address for the new vault to migrate to (if buyout is succesful)
interfaces/IMigration.sol:25:    // Boolean status to check if the propoal is active
interfaces/IMigration.sol:29:    // New fraction supply for a given vault that has succesfully migrated
modules/Migration.sol:322:        // Udpates ether balance of caller

2.3. Open TODOS

Consider resolving the TODOs before deploying.

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

2.4. Use string.concat() or bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>)) Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>))

FERC1155.sol:2:pragma solidity 0.8.13;
FERC1155.sol:395:                abi.encodePacked("\x19\x01", _domainSeparator, _structHash)
VaultFactory.sol:2:pragma solidity 0.8.13;
VaultFactory.sol:40:            .cloneCreationCode(abi.encodePacked());
VaultFactory.sol:47:            abi.encodePacked(bytes1(0xff), address(this), salt, creationHash)
VaultFactory.sol:68:        bytes memory data = abi.encodePacked();
VaultRegistry.sol:2:pragma solidity 0.8.13;
VaultRegistry.sol:32:            abi.encodePacked(msg.sender, address(this))
VaultRegistry.sol:154:            abi.encodePacked(_controller, address(this))

2.5. public functions not called by the contract should be declared external instead

 - MerkleBase.verifyProof(bytes32,bytes32[],bytes32) (src/utils/MerkleBase.sol#43-56)
 - Metadata.uri(uint256) (src/utils/Metadata.sol#39-41)
 - SelfPermit.selfPermit(address,uint256,bool,uint256,uint8,bytes32,bytes32) (src/utils/SelfPermit.sol#18-37)
 - SelfPermit.selfPermitAll(address,bool,uint256,uint8,bytes32,bytes32) (src/utils/SelfPermit.sol#46-63)

Overview

Table of Contents:

• 1. State variables only set in the constructor should be declared immutable
• 2. Reduce the size of error messages (Long revert Strings)
• 3. Use shift right/left instead of division/multiplication if possible
• 4. <array>.length should not be looked up in every loop of a for-loop
• 5. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 6. Increments/decrements can be unchecked in for-loops
• 7. Without the Optimizer: it costs more gas to initialize variables with their default value than letting the default value be applied
• 8. Use Custom Errors instead of Revert Strings to save Gas
• 9. Functions guaranteed to revert when called by normal users can be marked payable

1. State variables only set in the constructor should be declared immutable

Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)

modules/protoforms/BaseVault.sol:19:    address public registry;
modules/Buyout.sol:29:    address public registry;
modules/Buyout.sol:31:    address public supply;
modules/Buyout.sol:33:    address public transfer;
modules/Migration.sol:37:    address payable public buyout;
modules/Migration.sol:39:    address public registry;
modules/Minter.sol:14:    address public supply;
VaultFactory.sol:15:    address public implementation;

2. Reduce the size of error messages (Long revert Strings)

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.

Revert strings > 32 bytes:

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

Consider shortening the revert strings to fit in 32 bytes.

3. Use shift right/left instead of division/multiplication if possible

While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated, so the calculation can be unchecked in Solidity version 0.8+

• Use >> 1 instead of / 2
• Use >> 2 instead of / 4
• Use << 3 instead of * 8

Affected code (saves around 2 gas + 20 for unchecked per instance):

utils/MerkleBase.sol:100:                _node = _node / 2;
utils/MerkleBase.sol:136:                result = new bytes32[](length / 2 + 1);
utils/MerkleBase.sol:142:                result = new bytes32[](length / 2);

4. <array>.length should not be looked up in every loop of a for-loop

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, consider storing the array's length in a variable before the for-loop, and use this new variable instead:

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

5. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

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

Affected code:

utils/MerkleBase.sol:188:                ceil++;
Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
Vault.sol:104:        for (uint256 i = 0; i < length; i++) {

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

6. Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
modules/protoforms/BaseVault.sol:130:            for (uint256 i; i < _modules.length; ++i) {
modules/protoforms/BaseVault.sol:132:                for (uint256 j; j < leaves.length; ++j) {
modules/Migration.sol:496:            for (uint256 i; i < modulesLength; ++i) {
modules/Migration.sol:504:            for (uint256 i; i < modulesLength; ++i) {
modules/Migration.sol:507:                for (uint256 j; j < leavesLength; ++j) {
utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {
utils/MerkleBase.sol:110:            for (uint256 i; i < result.length; ++i) {
Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
Vault.sol:104:        for (uint256 i = 0; i < length; i++) {

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existant for uint256 here.

7. Without the Optimizer: it costs more gas to initialize variables with their default value than letting the default value be applied

If the optimizer is enabled, this finding isn't true anymore

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas (around 3 gas per instance).

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

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

Consider removing explicit initializations for default values.

8. Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

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

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

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).

Consider replacing all revert strings with custom errors in the solution.

utils/MerkleBase.sol:62:        require(_data.length > 1, "wont generate root for single leaf");
utils/MerkleBase.sol:78:        require(_data.length > 1, "wont generate proof for single leaf");
FERC1155.sol:263:        require(
FERC1155.sol:275:        require(
FERC1155.sol:297:        require(metadata[_id] != address(0), "NO METADATA");

9. Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyController is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

FERC1155.sol:198:    function setContractURI(string calldata _uri) external onlyController {