Nested Finance contest - TomFrenchBlockchain's results

Handle

TomFrench

Vulnerability details

Impact

NFTs can't be managed from future versions of NestedFactory without manual migration or removing support for the previous NestedFactory.

Proof of Concept

From discussion with NestedFinance team members, it's desired that multiple NestedFactories can interact with the NFT portfolios and be interoperable into the future.

NestedFinance has two singleton contracts which store the state of NFTs NestedAsset and NestedRecords

NestedAsset allows multiple factories to interact with a given NFT (asset) NestedRecords lists a single reserve which holds an NFT's assets (records)

This is fine however NestedFactory and NestedReserve are linked together 1:1 (factory, reserve)

This means that each NFT can only be managed by a single factory as calls from other factories to the relevant reserve will revert due to insufficient permissions. Should I want to update to use the newest factory I would have to manually migrate my portfolio across.

Recommended Mitigation Steps

Allow NestedReserve to have multiple factories connect to it. Make sure to have the NestedReserve secure from reentrancy attacks utilising multiple factories in parallel.

Handle

TomFrench

Vulnerability details

Impact

Gas costs

Proof of Concept

In the receive function of FeeSplitter we check that the address sending ETH is the WETH contract: https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L74

As we can safely say that the WETH contract will never send a metatransaction, we can just use msg.sender and avoid the extra gas costs of _msgSender()

Recommended Mitigation Steps

Replace _msgSender() with msg.sender

Handle

TomFrench

Vulnerability details

Impact

Reduced gas costs from not having to load operator address from storage and make external contract call to FlatOperator

Proof of Concept

FlatOperator is going to be used on almost any operation as it's very likely that people will be investing into a portfolio using one of the tokens in that portfolio. Despite having all the information necessary to return the correct output NestedFactory has to load the FlatOperator address from storage and then make a contract call to the FlatOperator (touching an additional contract) incurring all the associated costs of this.

As the FlatOperator is such a simple and commonly used operator it's worth inlining it into NestedFactory._submitOrder and NestedFactory._safeSubmitOrder.

_submitOrder could then be modified as similarly to this:

function _submitOrder(
  address _inputToken,
  address _outputToken,
  uint256 _nftId,
  Order calldata _order,
  bool _reserved
) private returns (uint256 amountSpent) {
  uint256[] memory amounts = new uint256[](2);
  address[] memory tokens = new address[](2);
  
  // Exact method of specifying when to trigger this is up to you.
  if (_order.operator == "Flat"){
   // Extract amount from calldata (you could also remove the token address from calldata)
   (, uint256 amount) = abi.decode(_order, (address, uint256))
   
   amounts[0] = amount;
   amounts[1] = amount;
  } else {
    address operator = requireAndGetAddress(_order.operator);
    (bool success, bytes memory data) = OperatorHelpers.callOperator(operator, _order.commit, _order.callData);
    require(success, "NestedFactory::_submitOrder: Operator call failed");

    (amounts, tokens) = OperatorHelpers.decodeDataAndRequire(
      data,
      _inputToken,
      _outputToken
    );
  }
 

  if (_reserved) {
    _transferToReserveAndStore(_outputToken, amounts[0], _nftId);
  }
  amountSpent = amounts[1];
}

We then shortcut this function saving gas.

Recommended Mitigation Steps

Something similar to above snippet.

Handle

TomFrench

Vulnerability details

Impact

NestedReserve.transferFromFactory is unused and so increases deployment costs for no gain

Proof of Concept

NestedReserve has a transferFromFactory which can be seen not to be used in the codebase (and in the case the NestedFactory needs to send tokens to the reserve it can do so directly.)

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedReserve.sol#L55-L60

Recommended Mitigation Steps

Remove this function.

Handle

TomFrench

Vulnerability details

Impact

Gas costs

Proof of Concept

In certain circumstances NestedFactory has to send some fees to the FeeSplitter

To do so it currently does:

1. Gives FeeSplitter an unlimited approval on the relevant token
2. Calls FeeSplitter.sendFees telling the FeeSplitter how much and which token has been paid in fees. a. Using this, FeeSplitter performs a transferFrom to take these funds from the NestedFactory

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

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L220-L235

This requires an SSTORE to set the unlimited approval and then all the relevant SLOADS and SSTORES in order to perform the transferFrom

Instead the NestedFactory could simply transfer the tokens to the FeeSplitter and then simply tell it how much has been paid in fees. This avoids all the costs associated with setting/reading approvals.

The downside is this either: a. makes FeeSplitter a permissioned function as it needs to trust the caller is a honest b. makes FeeSplitter need to perform some simple internal accounting when transferring tokens so that the difference between this and the contract's balance can be treated as the newly paid fees.

I'm fairly sure that option b turns out cheaper overall as fees are paid into the splitter much more regularly than they are paid out so increasing the costs of claiming for the benefit of paying is preferable.

Recommended Mitigation Steps

Consider a push pattern for fees with internal accounting of token balances.

Handle

TomFrench

Vulnerability details

Impact

Deployment + runtime gas cost increase

Proof of Concept

On each time we calculate the address of ZeroExStorage we hash the entirety of the creation code for ZeroExStorage. This means that not only do we have to perform a large hash operation over the entire creation bytecode of this contract, we need to store all of this bytecode in the ZeroExOperator's deployed bytecode.

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/operators/ZeroEx/ZeroExOperator.sol#L61

This hash could be calculated once at deployment and then have this used cheaply each time, reducing both deployment and runtime costs.

Recommended Mitigation Steps

Store keccak256(type(ZeroExStorage).creationCode) in an immutable (not constant as this still results in hashing being applied each time) variable.