Nested Finance contest - bobi's results

[L1] Check against same-token swap:

Assessed risk: 2/10

Urgency: N/A

Codebase frequency: 1

[L1 - Impact]:

Users would typically not swap against the same token, hence, developers must put in place a check against such swaps. Because of the rapid development and innovation that happens within the ETH smart contract ecosystem, one should never assume that what does not go wrong now will not go wrong in a few months, hence no exceptions are to be made when it comes to security and best practices.

function performSwap(
  IERC20 sellToken,
  IERC20 buyToken,
  bytes calldata swapCallData
) external payable override returns (uint256[] memory amounts, address[] memory tokens) {
    amounts = new uint256[](2);
    tokens = new address[](2);
    uint256 buyBalanceBeforePurchase = buyToken.balanceOf(address(this));
    uint256 sellBalanceBeforePurchase = sellToken.balanceOf(address(this));
		...

[L1 - References]:

operators/ZeroEx/ZeroExOperator.sol line 21

[L1 - Mitigation]:

Adding a require statement would prevent such a swap.

require(buyToken != sellToken, "ERR: same token swap");

[L2] Add _weight != 0 check in FeeSplitter

Assessed risk: 2/10

Urgency: N/A

Codebase frequency: 1

[L2 - Impact]:

Keeping a consistent check among all functions of the contract that deal with _weight is important, both from the security and clean code philosophies. One of the functions implemented in FeeSplitter.sol does not check against the latest _weight to be different than 0, contrary to all the similar functions that have this check.

function updateShareholder(uint256 _accountIndex, uint96 _weight) external onlyOwner {
    require(_accountIndex < shareholders.length, "FS: INVALID_ACCOUNT_INDEX");
    totalWeights = totalWeights + _weight - shareholders[_accountIndex].weight;
    require(totalWeights != 0, "FS: TOTAL_WEIGHTS_ZERO");
    shareholders[_accountIndex].weight = _weight;
    emit ShareholderUpdated(shareholders[_accountIndex].account, _weight);
}

[L2 - References]:

FeeSplitter.sol line 134

[L2 - Mitigation]:

Add a check against the _weight being 0.

require(_weight != 0, "FS: ZERO_WEIGHT");

[G1] Cache shareholders array.

[G1 - Details]:

Caching arrays is a powerful way of lowering the gas fees. Since in a for loop accessing memory array’s indexes would consume more gas than accessing the same array’s cached indexes as the length of the array becomes larger and larger, it is crucial to do the caching wherever possible.

[G1 - References]:

FeeSplitter.sol line 320 # (_addShareholder function)

[G1 - Mitigation]:

Cache the array in a similar way:

function _addShareholder(address _account, uint96 _weight) private {
    require(_weight != 0, "FS: ZERO_WEIGHT");
    require(_account != address(0), "FS: INVALID_ADDRESS");

    Shareholder[] memory shareholdersCache = shareholders;
    for (uint256 i = 0; i < shareholders.length; i++) {
        require(shareholdersCache[i].account != _account, "FS: ALREADY_SHAREHOLDER");
    }

    shareholders.push(Shareholder(_account, _weight));
    totalWeights += _weight;
    emit ShareholdersAdded(_account, _weight);
}