Nested Finance contest - WatchPug's results

Handle

WatchPug

Vulnerability details

safeApprove is deprecated.

See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/a94cf0509f19292c3edd3e102c025c793c241d05/contracts/token/ERC20/utils/SafeERC20.sol#L38-L44

Handle

WatchPug

Vulnerability details

Calling ERC20.transfer() without handling the returned value is unsafe.

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L271-L273

function unlockTokens(IERC20 _token) external override onlyOwner {
    _token.transfer(owner(), _token.balanceOf(address(this)));
}

Recommendation

Consider using OpenZeppelin's SafeERC20 library with safe versions of transfer functions.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L553-L559

/// @dev Calculate the fees for a specific user and amount (1%)
/// @param _user The user address
/// @param _amount The amount
/// @return The fees amount
function _calculateFees(address _user, uint256 _amount) private view returns (uint256) {
    return _amount / 100;
}

The function _calculateFees() is a rather simple function, replacing it with inline expression _amount / 100 can save some gas.

Handle

WatchPug

Vulnerability details

Contracts use assert() instead of require() in multiple places.

Per to Solidity’s documentation:

"Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.”

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L299-L299

assert(amountSpent <= _inputTokenAmount - feesAmount); // overspent

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L341-L341

assert(amountSpent <= _inputTokenAmounts[i]);

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/operators/ZeroEx/ZeroExOperator.sol#L42-L43

assert(amountBought > 0);
assert(amountSold > 0);

Recommendation

Change to require().

Handle

WatchPug

Vulnerability details

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• FeeSplitter.sol#_sendFees()

  https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/FeeSplitter.sol#L227-L227

Handle

WatchPug

Vulnerability details

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L84-L84

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

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L90-L90

require(address(reserve) == address(0), "NestedFactory::setReserve: Reserve is immutable");

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L97-L97

require(address(_feeSplitter) != address(0), "NestedFactory::setFeeSplitter: Invalid feeSplitter address");

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L109-L109

require(_orders.length > 0, "NestedFactory::create: Missing orders");

Handle

WatchPug

Vulnerability details

In FeeSplitter.sol#setRoyaltiesWeight(), totalWeights is being written 2 times. Combing them into one storage write can save gas.

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/FeeSplitter.sol#L94-L97

function setRoyaltiesWeight(uint256 _weight) public onlyOwner {
    totalWeights -= royaltiesWeight;
    royaltiesWeight = _weight;
    totalWeights += _weight;
}

Change to:

function setRoyaltiesWeight(uint256 _weight) public onlyOwner {
    totalWeights = totalWeights - royaltiesWeight + _weight;
    royaltiesWeight = _weight;
}

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/FeeSplitter.sol#L143-L146

function sendFees(IERC20 _token, uint256 _amount) external nonReentrant {
    uint256 weights = totalWeights - royaltiesWeight;
    _sendFees(_token, _amount, weights);
}

The local variable weights is used only once. Making the expression inline can save gas.

Recommendation

Change to:

function sendFees(IERC20 _token, uint256 _amount) external nonReentrant {
    _sendFees(_token, _amount, totalWeights - royaltiesWeight);
}

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedBuybacker.sol#L100-L100

uint256 balance = _sellToken.balanceOf(address(this));

balance is unused. Removing this line can save gas.

Handle

WatchPug

Vulnerability details

releaseToken() has nonReentrant modifier, making releaseTokens() to set storage _status multiple times in the for loop.

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/FeeSplitter.sol#L116-L129

function releaseToken(IERC20 _token) public nonReentrant {
    uint256 amount = _releaseToken(_msgSender(), _token);
    _token.safeTransfer(_msgSender(), amount);
    emit PaymentReleased(_msgSender(), address(_token), amount);
}

/// @notice Call releaseToken() for multiple tokens
/// @param _tokens ERC20 tokens to release
function releaseTokens(IERC20[] memory _tokens) external {
    for (uint256 i = 0; i < _tokens.length; i++) {
        releaseToken(_tokens[i]);
    }
}

Recommendation

Change to:

function releaseToken(IERC20 _token) public nonReentrant {
    _releaseTokenAndTransfer(_token);
}

/// @notice Call releaseToken() for multiple tokens
/// @param _tokens ERC20 tokens to release
function releaseTokens(IERC20[] memory _tokens) external {
    for (uint256 i = 0; i < _tokens.length; i++) {
        _releaseTokenAndTransfer(_tokens[i]);
    }
}

function _releaseTokenAndTransfer(IERC20 _token) private {
    uint256 amount = _releaseToken(_msgSender(), _token);
    _token.safeTransfer(_msgSender(), amount);
    emit PaymentReleased(_msgSender(), address(_token), amount);
}

Handle

WatchPug

Vulnerability details

The for loop in rebuildCache() will cost more gas if removeOperator() wont decrease operators.length.

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/MixinOperatorResolver.sol#L29-L43

function rebuildCache() public {
    bytes32[] memory requiredAddresses = resolverAddressesRequired();
    // The resolver must call this function whenever it updates its state
    for (uint256 i = 0; i < requiredAddresses.length; i++) {
        bytes32 name = requiredAddresses[i];
        // Note: can only be invoked once the resolver has all the targets needed added
        address destination = resolver.getAddress(name);
        if (destination != address(0)) {
            addressCache[name] = destination;
        } else {
            delete addressCache[name];
        }
        emit CacheUpdated(name, destination);
    }
}

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L78-L86

/// @inheritdoc INestedFactory
function removeOperator(bytes32 operator) external override onlyOwner {
    uint256 i = 0;
    while (operators[i] != operator) {
        i++;
    }
    require(i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator");
    delete operators[i];
}

Recommendation

Change to:

/// @inheritdoc INestedFactory
function removeOperator(bytes32 operator) external override onlyOwner {
    uint256 length = operators.length;
    for (uint256 i = 0; i < length; i++) {
        if (operators[i] == operator) {
            operators[i] = operators[length - 1];
            operators.pop();
            return;
        }
    }
    revert("NestedFactory::removeOperator: Cant remove non-existent operator");
}

Handle

WatchPug

Vulnerability details

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L285-L309

function _submitInOrders(
    uint256 _nftId,
    IERC20 _inputToken,
    uint256 _inputTokenAmount,
    Order[] calldata _orders,
    bool _reserved,
    bool _fromReserve
) private returns (uint256 feesAmount, IERC20 tokenSold) {
    _inputToken = _transferInputTokens(_nftId, _inputToken, _inputTokenAmount, _fromReserve);
    uint256 amountSpent;
    for (uint256 i = 0; i < _orders.length; i++) {
        amountSpent += _submitOrder(address(_inputToken), _orders[i].token, _nftId, _orders[i], _reserved);
    }
    feesAmount = _calculateFees(_msgSender(), amountSpent);
    assert(amountSpent <= _inputTokenAmount - feesAmount); // overspent

    // If input is from the reserve, update the records
    if (_fromReserve) {
        _decreaseHoldingAmount(_nftId, address(_inputToken), _inputTokenAmount);
    }

    _handleUnderSpending(_inputTokenAmount - feesAmount, amountSpent, _inputToken);

    tokenSold = _inputToken;
}

_inputTokenAmount - feesAmount at L306 will never underflow.

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/FeeSplitter.sol#L166-L174

function updateShareholder(uint256 _accountIndex, uint256 _weight) external onlyOwner {
    require(_accountIndex + 1 <= shareholders.length, "FeeSplitter: INVALID_ACCOUNT_INDEX");
    uint256 _totalWeights = totalWeights;
    _totalWeights -= shareholders[_accountIndex].weight;
    shareholders[_accountIndex].weight = _weight;
    _totalWeights += _weight;
    require(_totalWeights > 0, "FeeSplitter: TOTAL_WEIGHTS_ZERO");
    totalWeights = _totalWeights;
}

_accountIndex + 1 will never overflow.

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedBuybacker.sol#L107-L113

function trigger() internal {
    uint256 balance = NST.balanceOf(address(this));
    uint256 toBurn = (balance * burnPercentage) / 1000;
    uint256 toSendToReserve = balance - toBurn;
    _burnNST(toBurn);
    NST.safeTransfer(nstReserve, toSendToReserve);
}

balance - toBurn will never underflow.

Handle

WatchPug

Vulnerability details

For the storage variables that will be accessed multiple times, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).

For example:

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/FeeSplitter.sol#L152-L162

function sendFeesWithRoyalties(
    address _royaltiesTarget,
    IERC20 _token,
    uint256 _amount
) external nonReentrant {
    require(_royaltiesTarget != address(0), "FeeSplitter: INVALID_ROYALTIES_TARGET_ADDRESS");

    _sendFees(_token, _amount, totalWeights);
    _addShares(_royaltiesTarget, _computeShareCount(_amount, royaltiesWeight, totalWeights), address(_token));
}

Recommendation

Change to:

function sendFeesWithRoyalties(
    address _royaltiesTarget,
    IERC20 _token,
    uint256 _amount
) external nonReentrant {
    require(_royaltiesTarget != address(0), "FeeSplitter: INVALID_ROYALTIES_TARGET_ADDRESS");

    uint256 _totalWeights = totalWeights;

    _sendFees(_token, _amount, _totalWeights);
    _addShares(_royaltiesTarget, _computeShareCount(_amount, royaltiesWeight, _totalWeights), address(_token));
}

Handle

WatchPug

Vulnerability details

Unused local variables in contracts increase contract size and gas usage at deployment.

Instances include:

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L379-L383

(uint256[] memory amounts, address[] memory tokens) = OperatorHelpers.decodeDataAndRequire(
    data,
    _inputToken,
    _outputToken
);

tokens is unused.

https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L410-L414

(uint256[] memory amounts, address[] memory tokens) = OperatorHelpers.decodeDataAndRequire(
    data,
    _inputToken,
    _outputToken
);

tokens is unused.