Nested Finance contest - Omik's results

LOW: 1. Title : ETH may be stuck forever

Impact : In the https://code4rena.com/reports/2021-11-nested [M-08] the sponsored stated that this contract should not held any funds, and if there is any erc20 token accidentally send to this contract it is claimable by the owner through https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L132 unlockTokens function, since this contract has a receive https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L71 this contract can receive ETH, but there is no function to retrieve the ETH, therefore any ETH that was sent to this contract will be locked forever since the owner can't claim it also.

POC : https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L71

Mitigation : Add require to make this contract only receive etc from WETH address.

Title : missing validation between _inputTokenAmount and actual token that was inputed

Impact : The _transferInputTokens function is handling the token transfer from the user to this contract, however there is missing check whether the _inputTokenAmount is equal to the actual token that was transfer to this address, In the case AMP token, there is an external call when doing a transfer, an attacker can cause a mismatch when the token was transfer to this address, the attacker transfer the AMP token directly to this contract which inflating the balance of this address, therefore the difference between the _inputTokenAmount and the actual balance that this contract has, would be massive.

POC :

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L505

Title : There is no limit on how many operator that can be added

Impact : In the https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L100 the owner can add an operator, however there is no limit on how many operator can be added by the owner, if the operator inflated to a big value, the removeOperator would be failing on run out of gas, therefore the operator would be inflated and can't be removed.

POC : https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L100

GAS : 1. Title: Unnecessary owner function call

Impact: In the https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L134 it uses owner function call, instead of _owner, using _owner directly can save some gas

POC : https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L134

Title: It cheaper to remove the && operator and make the 2 different require

POC : https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedAsset.sol#L78

Mitigation :

require(_exists(_replicatedTokenId), "NA: INVALID_REPLICATED_TOKEN_ID");
require(tokenId != _replicatedTokenId, "NA: INVALID_REPLICATED_TOKEN_ID");

Title : It Cheaper to use 0 instead of empty string

Impact : change bytes32("") to bytes32(0) can save +- 3 gas, both check for zero bytes32

POC : https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L101

Mitigation :

require(operator != bytes32(0), "NF: INVALID_OPERATOR_NAME");

Title : It cheaper to cached the operators to a memory instead use storage multiple times

Impact : In the removeOperator function it do the check in the loop by calling the operators storage, instead checking it with memory, multiple storage read is more expansive than doing multiple read from memory therefore saving the operator value to a memory first before checking inside a loop can make this call cheaper, just like addOperator function do.

POC : https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L111

Mitigation :

function removeOperator(bytes32 operator) external override onlyOwner {
        bytes32[] memory operatorsCache = operators;
        uint256 operatorsLength = operatorsCache.length;
        for (uint256 i = 0; i < operatorsLength; i++) {
            if (operatorsCache[i] == operator) {
                operators[i] = operatorsCache[operatorsLength - 1];
                operators.pop();
                emit OperatorRemoved(operator);
                return;
            }
        }
        revert("NF: NON_EXISTENT_OPERATOR");
    }