Nested Finance contest - cmichel's results

Handle

cmichel

Vulnerability details

Some tokens (like USDT) don't correctly implement the EIP20 standard and their transfer/transferFrom function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

Non-safe transfers are used in:

• NestedFactory.unlockTokens

Impact

Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

Some parameters of functions are not checked for non-zero values:

• NestedFactory.constructor: address parameters could be zero or not a contract
• NestedReserve.constructor: address parameters could be zero or not a contract
• NestedBuybacker.constructor: address parameters could be zero or not a contract

Impact

Wrong user input or wallets defaulting to the zero addresses for a missing input can lead to the contract needing to redeploy or wasted gas.

Recommended Mitigation Steps

Validate the parameters.

Handle

cmichel

Vulnerability details

The NestedAsset.backfillTokenURI function can only be called by the factory but the factory never calls it.

Impact

Unused code can hint at programming or architectural errors. When minting tokens (using mint) it sets no tokenURI and there's currently no way to set it anymore.

Recommended Mitigation Steps

Add a way to use backfillTokenURI.

Handle

cmichel

Vulnerability details

The _transferInputTokens function accesses msg.value if the input token is ETH, and this function is called in a loop in _submitOutOrders. Therefore, in theory, one can specify two ETH orders but only pay once as the msg.value will be processed in each order iteration.

Impact

The severity is low as

1. calls to _transferInputTokens in a loop always have _fromReserve == true which does not access msg.value
2. the excess ETH still needs to already exist in the contract, which should usually not be the case as the swap amounts are either sent to the reserve or to the caller.

Recommended Mitigation Steps

I'd advocate against using msg.value in a loop. Imo, the best way to circumvent this is to not deal with ETH in the contract at all and just use WETH directly, as that's what the contract wants anyway for the orders. Maybe just adding a depositAllETH function that deposits msg.value to WETH once at the beginning of the swap functions makes sense.

Handle

cmichel

Vulnerability details

The NestedFactory.addOperator function pushes the operator even if it already exists in operators.

Impact

When this duplicated operator is removed through a removeOperator call, only the first instance is removed. The operator can now still be called which can lead to unexpected behavior.

Recommended Mitigation Steps

Check if the operator already exists before adding it.

Handle

cmichel

Vulnerability details

The NestedFactory._handleUnderSpending function implements a condition as _amountToSpent - _amountSpent > 0 instead of _amountToSpent > _amountSpent. The former reverts if _amountSpent > _amountToSpent while the latter doesn't.

It's unclear which behavior is preferred.

Recommended Mitigation Steps

Think about if _amountSpent > _amountToSpent should revert or not. If not, the if condition can be rewritten as _amountSpent > _amountToSpent which would also save gas.

Handle

cmichel

Vulnerability details

The NestedBuybacker.triggerForToken gets the _sellToken balance but does never use this result.

// this balance is never used
uint256 balance = _sellToken.balanceOf(address(this));

A good compiler should eliminate dead code but it's better to remove unused code in the first place.