Nested Finance contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/abstracts/MixinOperatorResolver.sol#L32-L46 https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/abstracts/MixinOperatorResolver.sol#L55

Vulnerability details

Impact

Currently, many core operations (like NestedFactory.create(), NestedFactory.swapTokenForTokens()) are dependent on the assumption that the cache is well synced before these functions are executed however this may not necessarily be the case.

Proof of Concept

The requireAndGetAddress method used in callOperator requires the operatorCache to be perfectly synchronized, or else it might make unwanted operator calls.

The rebuildCache method does not take into account that stale entries remain in the cache after being called with different results in the bytes32[] memory requiredOperators = resolverOperatorsRequired();

For example:

• resolverOperatorsRequired returns A,B,C and D
• call rebuildCache = all it's OK
• next time, because the contract's logic, resolverOperatorsRequired returns only A
• call rebuildCache = WRONG, it will retain in the cache A,B,C and D

The same problem exists in the isResolverCached method, it iterate through the current operators (requiredOperators), which in the case of being less (like empty), does not prevent the previously cached ones from remaining in the cache (and be called).

Note: The issue 217 related with a wrong cache was marked in the past as Medium

Recommended Mitigation Steps

Remove the old entries in rebuildCache and verify that the stored operators are exactly the same as you want (resolverOperatorsRequired).

This issue has been created to upgrade a QA report submission to a medium severity finding. From 0x1f8b:

The store method allows you to store the same token twice in NestedRecords.sol#L130 To do this you must first call store(_nftId,_tokenA,0,_reserve) and then store(_nftId,_tokenA,100000,_reserve) because it use records[_nftId].holdings[_token] as a flag and not checking if the amount is greater than zero, duplicate entries can be created, causing unwanted logics.

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/OperatorResolver.sol#L42-L43

Vulnerability details

Impact

The logic related to the areOperatorsImported method is incorrect and can cause an operator not to be updated because the owner thinks it is already updated, and a vulnerable or defective one can be used.

Proof of Concept

The operators mapping is made up of a key bytes32 name and a value made up of two values: implementation and selector, both of which identify the contract and function to be called when an operator is invoked.

The areOperatorsImported method tries to check if the operators to check already exist, however, the check is not done correctly, since && is used instead of ||.

If the operator with name A and value {implementation=0x27f8d03b3a2196956ed754badc28d73be8830a6e,selector="performSwapVulnerable"} exists, and the owner try to check if the operator with name A and value {implementation=0x27f8d03b3a2196956ed754badc28d73be8830a6e,selector="performSwapFixed"} exists, that function will return true, and the owner may decide not to import it , producing unexpected errors. Because operators manage the tokens, this error can produce a token lost.

Recommended Mitigation Steps

Change && by ||

GAS issues

1. Change the incremental logic from i++ to ++i in order to save some opcodes:

• FeeSplitter.sol#L126
• FeeSplitter.sol#L148
• FeeSplitter.sol#L165
• FeeSplitter.sol#L261
• FeeSplitter.sol#L280
• FeeSplitter.sol#L318
• NestedRecords.sol#L196
• OperatorResolver.sol#L40
• OperatorResolver.sol#L60
• OperatorResolver.sol#L75
• MixinOperatorResolver.sol#L36
• MixinOperatorResolver.sol#L55
• NestedFactory.sol#L103
• NestedFactory.sol#L113
• NestedFactory.sol#L153
• NestedFactory.sol#L213
• NestedFactory.sol#L273
• NestedFactory.sol#L291
• NestedFactory.sol#L327
• NestedFactory.sol#L369
• NestedFactory.sol#L581

2. The following structs could be optimized moving the position of certains values in order to save slot storages:

• Move fromReserve together inputToken
• Move toReserve together outputToken

3. Use delete instead of set to default value (false or address(0))

• OwnableFactoryHandler.sol#L37
• Delete if destination it's equal to address(0) at OperatorResolver.sol#L63

4. Change the logic in order to save one variable

• OwnableProxyDelegation.sol#L62-L66

    function _setOwner(address newOwner) private {
        emit OwnershipTransferred(_owner, newOwner);
        _owner = newOwner;
    }