Nested Finance contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

totalWeights might be zero and this will cause division by 0 runtime error. function _computeShareCount divides by _totalWeights:

  return (_amount * _weight) / _totalWeights;

Admin can accidentaly set totalWeights to 0 in function setRoyaltiesWeight when totalWeights = royaltiesWeight and _weight = 0.

Recommended Mitigation Steps

Consider algorithmitcally enforcing totalWeights > 0 or handle this special case in function _computeShareCount.

Handle

pauliax

Vulnerability details

Impact

function setReserve should check that _reserve address is not empty. function setNestedReserve should check that _nstReserve is not an empty address unless burning reserve tokens may be intended. function setFeeSplitter should check that _feeSplitter is not an empty address. function backfillTokenURI should validate that _tokenId exists, otherwise it will be possible to set metadata of tokens that are not minted yet.

Recommended Mitigation Steps

There are more functions that could enforce similar validations but I am not sure if you are interested in this or is this an intended behavior to not validate this.

Handle

pauliax

Vulnerability details

Impact

NestedRecords contains no removeFactory function so there is no way to revoke a factory in case you no longer want to support it. This function is present in NestedAsset contract so I thought you might want to also have it here.

Recommended Mitigation Steps

Consider if you are missing removeFactory or is this an intended functionality.

Handle

pauliax

Vulnerability details

Impact

function unlockTokens in NestedFactory uses a regular .transfer function while even an audit from Red4Sec suggested using safeTransfer from OZ library. This suggestion was implemented in other places but left untouched here.

Recommended Mitigation Steps

Consider also using safeTransfer here.

Handle

pauliax

Vulnerability details

Impact

function removeOperator has this check:

  (i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator");

However, 0 is actually a valid index of the array. It represents the first operator.

Recommended Mitigation Steps

A potential solution is to use the EnumerableSet to manage the operators: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/EnumerableSet.sol

Handle

pauliax

Vulnerability details

Impact

.length in a loop can be extracted into a variable and used where necessary to reduce the number of storage reads. Cache the length of the array and use this local variable when iterating over the storage array. This can be applied in functions findShareholder and _sendFees:

  for (uint256 i = 0; i < shareholders.length; i++)

  unit  shareholdersLength =  shareholders.length;
  for (uint256 i = 0; i < shareholdersLength; i++)

Handle

pauliax

Vulnerability details

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. There are many long revert messages across contracts, e.g.: "NestedRecords::increaseLockTimestamp: Can't decrease timestamp".

Handle

pauliax

Vulnerability details

Impact

setMaxAllowance is not very efficient. functions _transferFeeWithRoyalty and _handleUnderSpending invoke setMaxAllowance which does the job but my assumption is that it is not very efficient even when the allowance is a bit lower than a max uint value, but sufficient, it will still re-approve for max. This happens when approving a fee splitter in the functions mentioned above. Usually, it is a good practice not to approve more when the amount is known (and in this case it is), but if you want to optimize for gas, a one-time approval for max will work in your case as I assume fee splitter is a trusted contract. My recommendation is that you replace setMaxAllowance (or introduce a new function) that also skips the approval process when the current allowance is sufficient.

Handle

pauliax

Vulnerability details

Impact

Using the unchecked keyword to avoid redundant arithmetic underflow/overflow checks to save gas when an underflow/overflow cannot happen, e.g. the unchecked keyword can be applied in the following lines of code:

  if (_amountToSpent - _amountSpent > 0) {
    ...
    feeSplitter.sendFees(_token, _amountToSpent - _amountSpent); //unchecked
  }
  if (_inputTokenAmounts[i] - amountSpent > 0) {
    _transferFeeWithRoyalty(_inputTokenAmounts[i] - amountSpent, _inputToken, _nftId); //unchecked
  }

Handle

pauliax

Vulnerability details

Impact

This can be simplified to reduce gas costs by eliminating math operation:

  // before
  require(_accountIndex + 1 <= shareholders.length, "FeeSplitter: INVALID_ACCOUNT_INDEX");
  // after
  require(_accountIndex < shareholders.length, "FeeSplitter: INVALID_ACCOUNT_INDEX");

Handle

pauliax

Vulnerability details

Impact

There are places where the same storage variable is accessed multiple times in the same function. It would be more gas efficient to cache these variables and re-use them where necessary. E.g. function createRecord accesses records[_nftId].reserve 2 times in a require statement. function _transferToReserveAndStore accesses "reserve" 4 times. function isResolverCached accesses addressCache[name] 2 times.

Also, can save some gas here if you use local variables instead:

  emit BurnPartUpdated(burnPercentage);
  emit ReserveUpdated(nstReserve);
  emit FeeSplitterUpdated(feeSplitter);
  emit BurnPartUpdated(burnPercentage);

There might be more places where this suggestion can be applied. Consider caching repeated storage access to save some gas.

Handle

pauliax

Vulnerability details

Impact

I think it is not necessary to have function _burnNST as a separate private function. It is called only once and has just one LOC so it just incurs in extra gas cost which can be avoided by moving this line to function trigger and getting rid of _burnNST.

Handle

pauliax

Vulnerability details

Impact

function "mintWithMetadata" does not need onlyFactory modifier as it will be checked in function "mint" later.

Handle

pauliax

Vulnerability details

Impact

function triggerForToken calls ExchangeHelpers.fillQuote and continues the execution without checking the returned value. fillQuote returns a boolean flag indicating if the low-level call succeeded or failed.

Recommended Mitigation Steps

Consider adding a require statement that this fillQuote call succeeded.