Nested Finance contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedRecords.sol#L111

Vulnerability details

Impact

Duplicate tokens can be added which could exceed maxHoldingsCount and thus rejecting all functions in Factory

Proof of Concept

1. Observe the store function in NestedRecords.sol#L111

function store(
        uint256 _nftId,
        address _token,
        uint256 _amount,
        address _reserve
    ) external onlyFactory {
        uint256 amount = records[_nftId].holdings[_token];
        if (amount != 0) {
            require(records[_nftId].reserve == _reserve, "NRC: RESERVE_MISMATCH");
            updateHoldingAmount(_nftId, _token, amount + _amount);
            return;
        }
        require(records[_nftId].tokens.length < maxHoldingsCount, "NRC: TOO_MANY_TOKENS");
        require(
            _reserve != address(0) && (_reserve == records[_nftId].reserve || records[_nftId].reserve == address(0)),
            "NRC: INVALID_RESERVE"
        );

        records[_nftId].holdings[_token] = _amount;
        records[_nftId].tokens.push(_token);
        records[_nftId].reserve = _reserve;
    }

2. Factory calls the store function (via say _submitOrder function) with _nftId as 1, Token as X and amount as 0 (This is possible if a custom operator returns amounts[0] as 0 )

3. Since amount is 0 so below happens:

        records[1].holdings[X] = 0;
        records[1].tokens.push(X);

4. Now say Step 2 happens again that is store is again called with _nftId as 1, Token as X and amount as 0.

5. This time store mishandles the request since amount = records[_nftId].holdings[_token] =0 which means below statement will execute again

        records[1].holdings[X] = 0;
        records[1].tokens.push(X);

6. records[1].holdings[X] is harmless and will simply overwrite value but records[1].tokens.push(X); will add a duplicate token X which is incorrect

7. This becomes bigger problem once token length reaches maxHoldingsCount, making the factory unusable

Recommended Mitigation Steps

Make sure that token is not already existing before adding it

Missing token whitelisting puts stakeholders on risk

Contract: https://github.com/code-423n4/2022-02-nested/blob/main/contracts/FeeSplitter.sol#L146

1. Attacker can call sendFees with a malicious token contract

2. This increases the share balance of malicious token for each stake holder

3. When stakeholders tries to withdraw there share of malicious token using releaseTokens, malicious contract will be called and code written by attacker will be executed (asking for unauthorized approvals, wasting Gas etc)

sendFees & sendFeesWithRoyalties not handling ETH token

Contract: https://github.com/code-423n4/2022-02-nested/blob/main/contracts/FeeSplitter.sol#L175

1. Both sendFees & sendFeesWithRoyalties are not considering if the input _token is ETH as done in releaseTokens

Incorrect return message

Contract: https://github.com/code-423n4/2022-02-nested/blob/main/contracts/abstracts/MixinOperatorResolver.sol#L101

1. The require statement incorrectly mentions INVALID_OUTPUT_TOKEN when it should be INVALID_INPUT_TOKEN

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/FeeSplitter.sol (#L191, #L149, #L166)

1. In sendFeesWithRoyalties, releaseTokens, releaseTokensNoETH function, add check amount!=0

require(amount!=0, "Incorrect amount");

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedRecords.sol#L76

1. In updateHoldingAmount function, under the while loop, use return instead of break

if (tokens[tokenIndex] == _token) {
                    ...
                    return;
                }

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L317

1. No need to check require(batchLength != 0, "NF: INVALID_ORDERS"); as this is already checked in all the calling functions like create, _processInputOrders

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L445

1. The condition if (_amountToSpend > amounts[1]) is not required as the parent condition is already checking for require(amounts[1] <= _amountToSpend, "NestedFactory::_safeSubmitOrder: Overspent");