Nested Finance contest - elprofesor's results

Handle

elprofesor

Vulnerability details

Impact

The use of _token.transfer() in NestedFactory.unlockTokens may have unintended consequences. ERC20 tokens can implement contra to the EIP20 spec (USDT for instance returns VOID). This may result in tokens that return anything from false to void and these return values would not throw on failure. As a result transfer's in unlockTokens may not appropriately throw on failure.

Proof of Concept

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedFactory.sol#L271-L273

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer function that handles the return value check as well as non-standard-compliant tokens.

Handle

elprofesor

Vulnerability details

Impact

NestedFactory implements inappropriate validation preventing the removal of 0th index operator in operators array. This prevents the removal of any malicious or compromised operators which compromises could potentially break other functions within the NestedFactory contract.

Proof of Concept

Details listed above can be seen in the following:

require(i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator"); // @audit - LOW: can't remove 0'th element operator
        delete operators[i]; 

click here for link to git repo

Recommended Mitigation Steps

An example of more accurate removal process can be seen below:

/// @inheritdoc INestedFactory
function removeOperator(bytes32 operator) external override onlyOwner {
    uint256 i = 0;
    for (uint index=0 ; index < operators.length ; index++) {
        if(operators[i] == operator) i = index
    }
    require(i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator");
    delete operators[i];
}