Undesired behavior

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/69cf51d8e4eeb8bce3025db7f4f74cc565c9fad3/contracts/NestedRecords.sol#:~:text=uint256%20amount%20%3D%20records,_nftId%5D.reserve%20%3D%20_reserve%3B

Vulnerability details

You push a parameter into an array of tokens without checking if it's already exists. And if at first it's added with amount 0 it can later on be pushed with a greater amount and be twice in the array. Then in all processing it will consider the first occurrence and therefore the occurrence with amount 0.

     NestedRecords.store pushed the parameter _token

[maximebrugel]

Indeed, _amount is not checked and may result in the loss of funds for the user... If we only look at the store function.

However, this situation can't happen because of the `NestedFactory' (the only one able to call).

The Factory is calling with this private function :

/// @dev Transfer tokens to the reserve, and compute the amount received to store
/// in the records. We need to know the amount received in case of deflationary tokens.
/// @param _token The token to transfer (IERC20)
/// @param _amount The amount to send to the reserve
/// @param _nftId The Token ID to store the assets
function _transferToReserveAndStore(
    IERC20 _token,
    uint256 _amount,
    uint256 _nftId
) private {
    address reserveAddr = address(reserve);
    uint256 balanceReserveBefore = _token.balanceOf(reserveAddr);
    // Send output to reserve
    _token.safeTransfer(reserveAddr, _amount);
    uint256 balanceReserveAfter = _token.balanceOf(reserveAddr);
    nestedRecords.store(_nftId, address(_token), balanceReserveAfter - balanceReserveBefore, reserveAddr);
}

Here, the store amount parameter can be 0 if :

• _amount is equal to 0. Then balanceReserveAfter - balanceReserveBefore = 0.
• _amount is not equal to 0 but the safeTransfer function is transferring 0 tokens (100% fees, malicious contract,...).

We can't consider the second option, It is an external cause and we are not able to manage the exotic behaviors of ERC20s.
So, when the _amount parameter of this function can be equal to 0 ?

=> In submitOutOrders :

amountBought = _batchedOrders.outputToken.balanceOf(address(this));

(...)

amountBought = _batchedOrders.outputToken.balanceOf(address(this)) - amountBought;
feesAmount = amountBought / 100; // 1% Fee
if (_toReserve) {
    _transferToReserveAndStore(_batchedOrders.outputToken, amountBought - feesAmount, _nftId);
}

But the ZeroExOperator or FlatOperator will revert if the amount bought is 0.

=> In _submitOrder

(bool success, uint256[] memory amounts) = callOperator(_order, _inputToken, _outputToken);
require(success, "NF: OPERATOR_CALL_FAILED");
if (_toReserve) {
    _transferToReserveAndStore(IERC20(_outputToken), amounts[0], _nftId);
}

Same, the ZeroExOperator or FlatOperator will revert if the amount bought is 0.

In conclusion, we should check this parameter, but In the actual code state it can't happen (without taking into account the exotic ERC20s that we do not manage). If we add an operator that does not check the amount bought it can happen, so, maybe reducing the severity ?

[harleythedogC4]

Agree with sponsor, this issue doesn't exist with the current operators, so it is not currently a threat. I am going to downgrade this to medium and take #12 as the main issue.

[harleythedogC4]

Upon second thought I am making this the main issue.