Nested Finance contest - kenzo's results

Few small issues:

Wrong implementation of OperatorResolver::areOperatorsImported

The function as implemented will return true if the operators have same implementation but different selector, or different implementation but same selector. This might cause users/admins to think an upgrade has happened successfully when it fact it was not. Code ref:

            if (
                operators[names[i]].implementation != destinations[i].implementation &&
                operators[names[i]].selector != destinations[i].selector
            ) {
                return false;
            }

The condition should be ||, not &&.

Wrong revert reason in MixinOperatorResolver::callOperator

The second line should probably be "OH: INVALID_INPUT_TOKEN". Code ref:

            require(tokens[0] == _outputToken, "OH: INVALID_OUTPUT_TOKEN");
            require(tokens[1] == _inputToken, "OH: INVALID_OUTPUT_TOKEN");

Ether can be locked in NestedFactory

NestedFactory contains a general receive function, probably to facilitate unwrapping of WETH.

    receive() external payable {}

The function does not verify that the ETH sender is WETH contract. This means that ETH accidently sent from users will get locked in the contract. Consider adding an ETH rescue function, or verify that msg.sender == weth.

Two-step transferring of ownership

During the previous C4 audit, it was suggested that the ownership transferal could be changed to be two-step to insure no irreversible damage. Adrien responded "This is interesting. We will use this in the future but we're not ready to do our own implementation of the ownable library post-audit". As now you are using your own implementation of an ownable library, you can reconsider changing transferOwnership to be a two-step process. (although you've missed the audit, it's not complex functionality to add 🙂)

Remove unused ETH variable from FeeSplitter

FeeSplitter contains an ETH variable:

    address private constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;

It is unused and can be removed for cleanliness and for ~500 gas saved at deployment.

Some good words for the coding

I'd like to say that I'm quite impressed with the codebase. I don't know if I missed any big issues, but my impression of it is that the code is clean, clear and seems extra safe, even unusually so. You've taken precautions and I think that can pay off. A few times I would start thinking about a vector of attack to quickly discover that you have a require statement to account for that. So would just like to give a general thumbs up. 👍

Unused transfer function in NestedReserve

The transfer function in NestedReserve can be only called by NestedFactory, yet the factory doesn't use it. There are specific unit tests for this function but it is not used in the protocol. Removing it will save 61414 gas on deployment costs. Note: it is possible you are leaving it there for the future in case NestedFactory will be upgraded and you want this functionality to be possible. If so, this issue is invalid. But maybe it's just an old leftover.

Unused ETH variable in FeeSplitter

FeeSplitter contains an unused ETH variable:

    address private constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;

You can remove it for cleanliness and to save 500 gas at deployment.

Change NestedFactory's ETH variable to be immutable

By changing this variable in NestedFactory to immutable instead of constant:

    address private constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;

You will save 30-70 gas on various calls.