Fractional v2 contest - Sm4rty's results

1. MISSING ZERO-ADDRESS CHECK IN CONSTRUCTORS

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.

Proof of Concept

Navigate to the following all contract functions.

Buyout.sol Migration.sol Minter.sol BaseVault.sol SupplyReference.sol Supply.sol Metadata.sol

Recommended Mitigation Steps

Consider adding zero-address checks in the discussed constructors: require(newAddr != address(0));.

---

---

2. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

Instances:

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L24-L25

src/utils/MerkleBase.sol:24:            // TODO: This can be aesthetically simplified with a switch. Not sure it will
// save much gas but there are other optimizations to be had in here.

---

---

3. _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 IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.

Instances

src/FERC1155.sol:85:

src/FERC1155.sol:85:        _mint(_to, _id, _amount, _data);

Recommendations:

Use _safeMint() instead of _mint().

---

---

4. Unused receive() function will lock Ether in contract

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

Instances:

Links: src/modules/Buyout.sol:53 src/modules/Migration.sol:63 src/Vault.sol:32

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

Referencs

https://code4rena.com/reports/2022-05-sturdy#l-06-unused-receive-function-will-lock-ether-in-contract

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

This results in the keccak operation being performed whenever the variable is used, increasing gas costs relative to just storing the output hash. Changing to immutable will only perform hashing on contract deployment which will save gas.

Instances:

src/constants/Permit.sol

src/constants/Permit.sol:5:bytes32 constant DOMAIN_TYPEHASH = keccak256(
src/constants/Permit.sol:10:bytes32 constant PERMIT_TYPEHASH = keccak256(
src/constants/Permit.sol:15:bytes32 constant PERMIT_ALL_TYPEHASH = keccak256(

References:

https://github.com/code-423n4/2021-10-slingshot-findings/issues/3

---

---

2. An array’s length should be cached to save gas in for-loops

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

Instances:

Links: src/modules/Buyout.sol:454 src/modules/protoforms/BaseVault.sol:64 src/modules/protoforms/BaseVault.sol:83 src/modules/protoforms/BaseVault.sol:107 src/modules/protoforms/BaseVault.sol:130 src/modules/protoforms/BaseVault.sol:132 src/utils/MerkleBase.sol:51 src/utils/MerkleBase.sol:110

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

Remediation:

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

---

---

3. IT COSTS MORE GAS TO INITIALIZE VARIABLES TO ZERO THAN TO LET THE DEFAULT OF ZERO BE APPLIED

Instances:

Links src/modules/protoforms/BaseVault.sol:64 src/modules/protoforms/BaseVault.sol:83 src/modules/protoforms/BaseVault.sol:107 src/utils/MerkleBase.sol:51 src/Vault.sol:78 src/Vault.sol:104

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/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++) {

Reference

https://code4rena.com/reports/2022-04-phuture/#g-11-it-costs-more-gas-to-initialize-variables-to-zero-than-to-let-the-default-of-zero-be-applied

---

---

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

Instances:

Links: src/utils/MerkleBase.sol:62 src/utils/MerkleBase.sol:78

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");

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.****

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 Custom Errors in Solidity:

---

---

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

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 Includes:

Links: src/FERC1155.sol:263-268 src/FERC1155.sol:275-286 src/utils/MerkleBase.sol:62 src/utils/MerkleBase.sol:78

src/FERC1155.sol:263-268
263:        require(
...            msg.sender == _from ||
...            isApprovedForAll[_from][msg.sender] ||
...               isApproved[_from][msg.sender][_id], 
267:"NOT_AUTHORIZED");

src/FERC1155.sol:275-286
  275:         require(
  ...              _to.code.length == 0
  ...
  ...               ) == INFTReceiver.onERC1155Received.selector,
  286:              "UNSAFE_RECIPIENT");

src/FERC1155.sol:298
  298:         require(metadata[_id] != address(0), "NO METADATA");

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

References:

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

Remediation:

I suggest replacing revert strings with custom errors.

---

---

6. Shift Right instead of Dividing by 2

A division by 2 can be calculated by shifting one to the right.

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.

I suggest replacing / 2 with >> 1 here:

Instances:

Github Links: src/utils/MerkleBase.sol:100 src/utils/MerkleBase.sol:136 src/utils/MerkleBase.sol:142

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

References:

https://code4rena.com/reports/2022-04-badger-citadel/#g-32-shift-right-instead-of-dividing-by-2

---

---

7. ++i costs less gas than ++i, especially when it’s used in for-loops (--i/i-- too

++i costs less gas as compared to i++ for unsigned integer, as per-increment is cheaper.

Instances.

src/FERC1155.sol src/Vault.sol: src/modules/Migration.sol: src/modules/protoforms/BaseVault.sol: src/utils/MerkleBase.sol:

src/FERC1155.sol:
  339:                     nonces[_owner]++,
  363:                     nonces[_owner]++,

src/Vault.sol:
   78:         for (uint256 i = 0; i < length; i++) {
  104:         for (uint256 i = 0; i < length; i++) {

src/modules/Migration.sol:
  508:                     hashes[counter++] = leaves[j];

src/modules/protoforms/BaseVault.sol:
  133:                     hashes[counter++] = leaves[j];

src/utils/MerkleBase.sol:
  188:                 ceil++;

Reference:

https://betterprogramming.pub/stop-using-i-in-your-loops-1f906520d548 https://code4rena.com/reports/2022-05-cally#g-19-i-costs-less-gas-than-i-especially-when-its-used-in-for-loops---ii---too