Nested Finance contest - 0x0x0x's results

Handle

0x0x0x

Vulnerability details

Proof of Concept

For unsigned integers, it is cheaper to check != 0 than > 0. Both provide the same logic.

Occurences

contracts/FeeSplitter.sol:105:        require(_accounts.length > 0 && _accounts.length == _weights.length, "FeeSplitter: ARRAY_LENGTHS_ERR");
contracts/FeeSplitter.sol:172:        require(_totalWeights > 0, "FeeSplitter: TOTAL_WEIGHTS_ZERO");
contracts/FeeSplitter.sol:263:        require(_weight > 0, "FeeSplitter: ZERO_WEIGHT");
contracts/NestedBuybacker.sol:97:        if (feeSplitter.getAmountDue(address(this), _sellToken) > 0) {
contracts/NestedFactory.sol:69:        require(i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator");
contracts/NestedFactory.sol:94:        require(_orders.length > 0, "NestedFactory::create: Missing orders");
contracts/NestedFactory.sol:110:        require(_orders.length > 0, "NestedFactory::addTokens: Missing orders");
contracts/NestedFactory.sol:124:        require(_orders.length > 0, "NestedFactory::swapTokenForTokens: Missing orders");
contracts/NestedFactory.sol:143:        require(_orders.length > 0, "NestedFactory::sellTokensToNft: Missing orders");
contracts/NestedFactory.sol:163:        require(_orders.length > 0, "NestedFactory::sellTokensToWallet: Missing orders");
contracts/NestedFactory.sol:194:        require(_orders.length > 0, "NestedFactory::destroy: Missing orders");
contracts/NestedFactory.sol:333:            if (_inputTokenAmounts[i] - amountSpent > 0) {
contracts/NestedFactory.sol:467:        if (_amountToSpent - _amountSpent > 0) {
contracts/NestedRecords.sol:171:        require(_maxHoldingsCount > 0, "NestedRecords: INVALID_MAX_HOLDINGS");
contracts/operators/Flat/FlatOperator.sol:18:        require(amount > 0, "FlatOperator::commitAndRevert: Amount must be greater than zero");
contracts/operators/ZeroEx/ZeroExOperator.sol:42:        assert(amountBought > 0);
contracts/operators/ZeroEx/ZeroExOperator.sol:43:        assert(amountSold > 0);

Handle

0x0x0x

Vulnerability details

Explanation

updateShareholder in FeeSplitter.sol can be implemented more efficiently. The updated version consumes less gas and also has the second require statement earlier, which reduces the gas cost in case the statement of second require is not fullfilled.

FeeSplitter.sol : L166-174:

    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;
    }

can be replaced with:

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

Tools Used

Manual analysis

Handle

0x0x0x

Vulnerability details

Proof of Concept

Example:

for (uint i = 0; i < arr.length; i++) {
//Operations not effecting the length of the array.
}

Loading length of array costs gas. Therefore, the length should be cached, if the length of the array doesn't change inside the loop. Recommended implementation:

uint length = arr.length;
for (uint i = 0; i < length; i++) {
//Operations not effecting the length of the array.
}

By doing so the length is only loaded once rather than loading it as many times as iterations (Therefore, less gas is spent).

Occurences

contracts/FeeSplitter.sol:108:        for (uint256 i = 0; i < _accounts.length; i++) {
contracts/FeeSplitter.sol:125:        for (uint256 i = 0; i < _tokens.length; i++) {
contracts/FeeSplitter.sol:210:        for (uint256 i = 0; i < shareholders.length; i++) {
contracts/FeeSplitter.sol:227:        for (uint256 i = 0; i < shareholders.length; i++) {
contracts/MixinOperatorResolver.sol:32:        for (uint256 i = 0; i < requiredAddresses.length; i++) {
contracts/MixinOperatorResolver.sol:48:        for (uint256 i = 0; i < requiredAddresses.length; i++) {
contracts/NestedFactory.sol:203:        for (uint256 i = 0; i < tokens.length; i++) {
contracts/NestedFactory.sol:280:        for (uint256 i = 0; i < _orders.length; i++) {
contracts/NestedFactory.sol:316:        for (uint256 i = 0; i < _orders.length; i++) {
contracts/OperatorResolver.sol:33:        for (uint256 i = 0; i < names.length; i++) {
contracts/OperatorResolver.sol:45:        for (uint256 i = 0; i < names.length; i++) {
contracts/OperatorResolver.sol:56:        for (uint256 i = 0; i < destinations.length; i++) {