QA Report

[code423n4]

QA Report

Table of Contents:

• QA Report
  • Foreword
  • Comparisons
    • Comparison that should be inclusive in NestedRecords.sol
  • Variables
    • Missing Address(0) checks
    • Check if a value is in an array before a push
    • Variables that should be grouped together in a struct
      • File: FeeSplitter.sol
  • Arithmetics
    • Possible division by 0
  • Revert Strings
    • File: NestedFactory.sol
      • Inconsistent Revert string
    • File: MixinOperatorResolver.sol
      • Inconsistent Revert string (1)
      • Misleading + Inconsistent Revert string (2)
    • File: OwnableProxyDelegation.sol
      • Inconsistent Revert string (1)
      • Inconsistent Revert string (2)
    • File: ZeroExOperator.sol
      • Inconsistent Revert string (1)
      • Inconsistent Revert string (2)
  • Comments
    • File: NestedFactory.sol
      • Missing comment "@return" (1)
      • Missing comment "@return" (2)
      • Missing comment "@param" (1)
    • File: NestedRecords.sol
      • Missing comment "@return" (1)
      • Missing comment "@return" (2)
      • Misleading comment on "@return"
    • File: MixinOperatorResolver.sol
      • Missing 2 comments "@param" & changeable "@return" comment/variable
    • File: ExchangeHelpers.sol
      • Missing comment "@return"

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Comparisons

Comparison that should be inclusive in NestedRecords.sol

File: NestedRecords.sol
123:         require(records[_nftId].tokens.length < maxHoldingsCount, "NRC: TOO_MANY_TOKENS"); //@audit should be inclusive

As length isn't 0 indexed, I believe, as an example to illustrate, that if maxHoldingsCount == 1, then records[_nftId].tokens.length == 1 should be a passing condition. Therefore, I suggest changing < with <=

Variables

Missing Address(0) checks

File: MixinOperatorResolver.sol
22:     constructor(address _resolver) {
23:         resolver = OperatorResolver(_resolver); //@audit missing address(0) check on immutable just like in the constructors in FeeSplitter.sol and NestedFactory.sol
24:     }

Check if a value is in an array before a push

In NestedRecords.sol's store function, it's possible to push an existing address _token several times in the same array

File: NestedRecords.sol
130:         records[_nftId].tokens.push(_token); //@audit : should check existence

The previous lines of codes don't prevent this.
The store function has the modifier onlyFactory and the only impact seem to be a possible maximization of records[_nftId].tokens.length (so that it reaches maxHoldingsCount).

Variables that should be grouped together in a struct

For maps that use the same key value: having separate fields is error prone (like in case of deletion or future new fields).
By regrouping, it's then possible to delete all related fields with a simple delete newStruct[previousSameKeyForAllPreviousMaps].

File: FeeSplitter.sol

2 maps can be grouped together, as they use the same _account key:

62:     struct TokenRecords {
63:         uint256 totalShares;
64:         uint256 totalReleased;
65:         mapping(address => uint256) shares; //@audit group 
66:         mapping(address => uint256) released; //@audit group 
67:     }

I'd suggest these 2 related data get grouped in a struct, let's name it AccountInfo:

struct AccountInfo {   
    uint256 shares;   
    uint256 released;   
}   

And it would be used in this manner (where address is _account):

    struct TokenRecords {
        uint256 totalShares;
        uint256 totalReleased;
        mapping(address => AccountInfo) accountInfo;
    }   

Arithmetics

Possible division by 0

There are no checks that the denominator is != 0 here:

File: FeeSplitter.sol
327:     function _computeShareCount(
328:         uint256 _amount,
329:         uint256 _weight,
330:         uint256 _totalWeights
331:     ) private pure returns (uint256) {
332:         return (_amount * _weight) / _totalWeights; // @audit _totalWeights can be equal to 0, see FeeSplitter.sol:L184
333:     }

Revert Strings

File: NestedFactory.sol

Inconsistent Revert string

44:         require(_exists(_tokenId), "URI query for nonexistent token");

All other revert strings in NestedAsset.sol begin with NA: . Only this one doesn't. It's possible to gain consistency and still have an < 32 bytes size string with the following: "NA: URI query - inexistent token"

File: MixinOperatorResolver.sol

Inconsistent Revert string (1)

100:             require(tokens[0] == _outputToken, "OH: INVALID_OUTPUT_TOKEN");//@audit LOW comment : MOR like above

Here, "OH: INVALID_OUTPUT_TOKEN" should be replaced with "MOR: INVALID_OUTPUT_TOKEN"

Misleading + Inconsistent Revert string (2)

101:             require(tokens[1] == _inputToken, "OH: INVALID_OUTPUT_TOKEN"); //@audit LOW comment : INVALID_INPUT_TOKEN //@audit LOW comment : MOR

Here, "OH: INVALID_OUTPUT_TOKEN" should be replaced with "MOR: INVALID_INPUT_TOKEN"

File: OwnableProxyDelegation.sol

Inconsistent Revert string (1)

25:         require(!initialized, "OFP: INITIALIZED"); //@audit low OFP doesn't make sense, use OPD instead (example: OwnableFactoryHandler is OFH, MixinOperatorResolver is MOR)

Is most contracts, the capital letters from the contract's name are used as a prefix in the revert strings (OwnableFactoryHandler has OFH, MixinOperatorResolver has MOR). Here, OFP doesn't really reflect OwnableProxyDelegation. It should be OPD.

Inconsistent Revert string (2)

26:         require(StorageSlot.getAddressSlot(_ADMIN_SLOT).value == msg.sender, "OFP: FORBIDDEN");//@audit should be "OPD: FORBIDDEN" 

Same as above: OFP should be OPD.

File: ZeroExOperator.sol

Inconsistent Revert string (1)

32:         require(success, "ZEO: SWAP_FAILED");
...
36:         require(amountBought != 0, "ZeroExOperator::performSwap: amountBought cant be zero"); //@audit LOW do like line 32 : "ZEO: amountBought cant be zero" < 32 bytes & consistent

As said before, the capital letters from the contract's name are used as a prefix in the revert strings. Here, the revert string's size is > 32 bytes and isn't using the same style as 4 lines above it. ZeroExOperator::performSwap should be ZEO.

Inconsistent Revert string (2)

32:         require(success, "ZEO: SWAP_FAILED");
...
37:         require(amountSold != 0, "ZeroExOperator::performSwap: amountSold cant be zero");//@audit do like line 32 : "ZEO: amountSold cant be zero" < 32 bytes & consistent

Same as above: ZeroExOperator::performSwap should be ZEO.

Comments

File: NestedFactory.sol

Missing comment "@return" (1)

403:     /// @dev Call the operator to submit the order and add the output
404:     /// assets to the reserve (if needed).
405:     /// @param _inputToken Token used to make the orders
406:     /// @param _outputToken Expected output token
407:     /// @param _nftId The nftId
408:     /// @param _order The order calldata
409:     /// @param _toReserve True if the output is store in the reserve/records, false if not. //@audit missing @return 
410:     function _submitOrder(
411:         address _inputToken,
412:         address _outputToken,
413:         uint256 _nftId,
414:         Order calldata _order,
415:         bool _toReserve
416:     ) private returns (uint256 amountSpent) {

Missing comment "@return" (2)

474:     /// @dev Choose between ERC20 (safeTransfer) and ETH (deposit), to transfer from the Reserve
475:     ///      or the user wallet, to the factory.
476:     /// @param _nftId The NFT id
477:     /// @param _inputToken The token to receive
478:     /// @param _inputTokenAmount Amount to transfer
479:     /// @param _fromReserve True to transfer from the reserve
480:     /// @return Token transfered (in case of ETH)
481:     ///         The real amount received after the transfer to the factory //@audit missing @return (not the description, just the keyword)
482:     function _transferInputTokens(
483:         uint256 _nftId,
484:         IERC20 _inputToken,
485:         uint256 _inputTokenAmount,
486:         bool _fromReserve
487:     ) private returns (IERC20, uint256) {

Missing comment "@param" (1)

562:     /// @dev Transfer from factory and collect fees
563:     /// @param _token The token to transfer
564:     /// @param _amount The amount (with fees) to transfer
565:     /// @param _dest The address receiving the funds //@audit missing @param
566:     function _safeTransferWithFees(
567:         IERC20 _token,
568:         uint256 _amount,
569:         address _dest,
570:         uint256 _nftId
571:     ) private {

File: NestedRecords.sol

Missing comment "@return" (1)

162:     /// @param _nftId The id of the NFT> //@audit missing @return
163:     function getAssetTokens(uint256 _nftId) public view returns (address[] memory) {

Missing comment "@return" (2)

183:     /// @param _token The address of the token //@audit missing @return
184:     function getAssetHolding(uint256 _nftId, address _token) public view returns (uint256) {

Misleading comment on "@return"

Here, the comment @return The holdings, which is the unique @return comment, suggests a returned mapping(address => uint256) holdings as seen on struct NftRecord. However, the function is actually returning a uint256[] and an address[]. Therefore, two @return are required and the previous one should be deleted.

Code:

188:     /// @notice Returns the holdings associated to a NestedAsset
189:     /// @param _nftId the id of the NestedAsset
190:     /// @return The holdings //@audit "The holdings" suggests a "mapping(address => uint256)" but a uint256[] and an address[] are returned.  
191:     function tokenHoldings(uint256 _nftId) public view returns (address[] memory, uint256[] memory) {

File: MixinOperatorResolver.sol

Missing 2 comments "@param" & changeable "@return" comment/variable

    /// @dev Build the calldata (with safe datas) and call the Operator
    /// @param _order The order to execute //@audit missing @param _inputToken and @param _outputToken
    /// @return success If the operator call is successful
    /// @return amounts The amounts from the execution (used and received) //@audit why not use uint256[2]?
    ///         - amounts[0] : The amount of output token
    ///         - amounts[1] : The amount of input token USED by the operator (can be different than expected)
    function callOperator(
        INestedFactory.Order calldata _order,
        address _inputToken,
        address _outputToken
    ) internal returns (bool success, uint256[] memory amounts) {

I suggest changing the returned uint256[] to uint256[2]

File: ExchangeHelpers.sol

Missing comment "@return"

10:     /// @dev Perform a swap between two tokens
11:     /// @param _sellToken Token to exchange
12:     /// @param _swapTarget The address of the contract that swaps tokens
13:     /// @param _swapCallData Call data provided by 0x to fill the quote //@audit missing @return
14:     function fillQuote(
15:         IERC20 _sellToken,
16:         address _swapTarget,
17:         bytes memory _swapCallData
18:     ) internal returns (bool) {

[maximebrugel]

Comparisons

Comparison that should be inclusive in NestedRecords.sol (Disputed)

At the moment of storing, the comparison should be inclusive. If not, we are exceeding the maxHoldingsCount amount.

Variables

Missing Address(0) checks (Confirmed)

Check if a value is in an array before a push (Duplicated)

#6

Variables that should be grouped together in a struct (Acknowledged)

I prefer to keep it simple.

Arithmetics

Possible division by 0 (Disputed)

_totalWeights = 0 should not happen, and if it’s the case a panic error is fine.

Revert Strings

MixinOperatorResolver: Inconsistent Revert string (Confirmed)

MixinOperatorResolver : Misleading + Inconsistent Revert string (Duplicated)

#9

OwnableProxyDelegation: Inconsistent Revert string 1 & 2 (Confirmed)

ZeroExOperator: Inconsistent Revert string 1 & 2 (Confirmed)

Comments

NestedFactory: Missing comment 1 & 3 (Confirmed)

NestedFactory: Missing comment 2 (Disputed)

The second return param comment is here.

NestedRecords: Missing comment 1 & 2 (Confirmed)

NestedRecords: Misleading comment on « @return » (Confirmed)

MixinOperatorResolver: Missing comments 1 & 2 (Confirmed)

[harleythedogC4]

My personal judgements:

1. "Comparison that should be inclusive in NestedRecords.sol". Length is taken before the push. Invalid.
2. "Missing Address(0) checks". Valid and very-low-critical.
3. "Check if a value is in an array before a push". This has been upgraded to medium severity in Check if a value is in an array before a push #79. Will not contribute to the QA score.
4. "Variables that should be grouped together in a struct". Valid and non-critical.
5. "Possible division by 0". Agree with sponsor, the transaction will revert on division by zero anyways. Invalid.
6. "Revert Strings". Agree with all. Valid and non-critical (x4).
7. "Comments". Agree with all of the warden's submissions, including the disputed one. The warden noted that the "@return" part of NatSpec was missing for the second argument. Valid and non-critical (x5)

[harleythedogC4]

Adding in the reduced severity finding #59:
8. "Destroy can avoid the bulk of fees". I will assign this as Valid and low-critical.

[harleythedogC4]

Now, here is the methodology I used for calculating a score for each QA report. I first assigned each submission to be either non-critical (1 point), very-low-critical (5 points) or low-critical (10 points), depending on how severe/useful the issue is. The score of a QA report is the sum of these points, divided by the maximum number of points achieved by a QA report. This maximum number was 26 points, achieved by #66.

The number of points achieved by this report is 25 points.
Thus the final score of this QA report is (25/26)*100 = 96.