Platform: Code4rena
Start Date: 10/02/2022
Pot Size: $30,000 USDC
Total HM: 5
Participants: 24
Period: 3 days
Judge: harleythedog
Total Solo HM: 3
Id: 86
League: ETH
Rank: 7/24
Findings: 3
Award: $1,474.87
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: robee
Also found by: 0x1f8b, csanuragjain
1024.836 USDC - $1,024.84
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
#0 - maximebrugel
2022-02-10T10:14:39Z
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 ?
#1 - harleythedogC4
2022-02-27T06:29:13Z
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.
#2 - harleythedogC4
2022-02-27T16:44:09Z
Upon second thought I am making this the main issue.
105.3884 USDC - $105.39
Title: Does not validate the input fee parameter Severity: Low Risk
Some fee parameters of functions are not checked for invalid values. Validate the parameters:
NestedFactory.constructor (_feeSplitter) NestedFactory.setFeeSplitter (_feeSplitter)
Title: Solidity compiler versions mismatch Severity: Low Risk
The project is compiled with different versions of solidity, which is not recommended due ti undefined behaviors as a result of it.
Title: Init function calls an owner function Severity: Low Risk
Init function that calls an onlyOwner function is problematic since sometimes the initializer or the one applies the constructor isn't necessary the owner of the protocol. And if a contract does it then you might get a situation that all the onlyOwner functions are blocked since only the factory contract may use them but isn't necessary support it. FeeSplitter.sol.constructor - calls setRoyaltiesWeight FeeSplitter.sol.constructor - calls setShareholders
Title: Not verified owner Severity: Low Risk
owner param should be validated to make sure the owner address is not address(0). Otherwise if not given the right input all only owner accessible functions will be unaccessible. OwnableProxyDelegation.sol.transferOwnership newOwner OwnableProxyDelegation.sol.initialize ownerAddr NestedAsset.sol.burn _owner NestedAsset.sol.mint _owner NestedAsset.sol.backfillTokenURI _owner NestedAsset.sol.mintWithMetadata _owner OwnableProxyDelegation.sol._setOwner newOwner
Title: Two Steps Verification before Transferring Ownership Severity: Low Risk
The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105
OwnableProxyDelegation.sol
Title: Missing non reentrancy modifier Severity: Low Risk
The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..
FeeSplitter.sol, updateShareholder is missing a reentrancy modifier NestedFactory.sol, receive is missing a reentrancy modifier FeeSplitter.sol, receive is missing a reentrancy modifier NestedFactory.sol, removeOperator is missing a reentrancy modifier NestedFactory.sol, addOperator is missing a reentrancy modifier NestedFactory.sol, setFeeSplitter is missing a reentrancy modifier FeeSplitter.sol, setShareholders is missing a reentrancy modifier NestedFactory.sol, unlockTokens is missing a reentrancy modifier FeeSplitter.sol, setRoyaltiesWeight is missing a reentrancy modifier NestedFactory.sol, updateLockTimestamp is missing a reentrancy modifier
Title: In the following public update functions no value is returned Severity: Low Risk
In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).
ZeroExStorage.sol, updatesSwapTarget NestedRecords.sol, updateHoldingAmount NestedFactory.sol, updateLockTimestamp NestedRecords.sol, updateLockTimestamp FeeSplitter.sol, updateShareholder
Title: Never used parameters Severity: Low Risk
Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.
MockERC20.sol: function constructor parameter _symbol isn't used. (constructor is default) NestedAsset.sol: function backfillTokenURI parameter _owner isn't used. (backfillTokenURI is external) TestableMixingOperatorResolver.sol: function constructor parameter _resolver isn't used. (constructor is default) DeflationaryMockERC20.sol: function constructor parameter _symbol isn't used. (constructor is default) DeflationaryMockERC20.sol: function constructor parameter _name isn't used. (constructor is default) TestableOperatorCaller.sol: function performSwap parameter own isn't used. (performSwap is external) MockERC20.sol: function constructor parameter _name isn't used. (constructor is default)
Title: Missing commenting Severity: Low Risk
The following functions are missing commenting as describe below: FeeSplitter.sol, _addShareholder (private), parameters _account, _weight not commented NestedRecords.sol, getAssetTokens (public), @return is missing FeeSplitter.sol, _releaseToken (private), @return is missing FeeSplitter.sol, _computeShareCount (private), parameters _amount, _weight, _totalWeights not commented NestedRecords.sol, getAssetHolding (public), @return is missing FeeSplitter.sol, _releaseToken (private), parameters _account, _token not commented FeeSplitter.sol, _computeShareCount (private), @return is missing
Title: Anyone can withdraw others Severity: Low Risk
Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still 1) not the desired behavior 2) can be dangerous if the receiver is a smart contract 3) the receiver may not know someone withdraw him
NestedFactory.withdraw NestedReserve.withdraw
Title: Not verified input Severity: Low Risk
external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.
FeeSplitter.sol._addShares _token FeeSplitter.sol._addShares _account DeflationaryMockERC20.sol.transferFrom recipient OwnableFactoryHandler.sol.removeFactory _factory
#0 - maximebrugel
2022-02-10T14:10:34Z
NestedFactory.constructor, the _feeSplitter param is checked:require( address(_nestedAsset) != address(0) && address(_nestedRecords) != address(0) && address(_reserve) != address(0) && address(_feeSplitter) != address(0) && // Checked address(_weth) != address(0) && _operatorResolver != address(0), "NF: INVALID_ADDRESS"
NestedFactory.setFeeSplitter, the _feeSplitter param is checked:require(address(_feeSplitter) != address(0), "NF: INVALID_FEE_SPLITTER_ADDRESS");
All the files in the scope (excluding mocks) are in Solidity 0.8.11.
The FeeSplitter is not deployed by a smart contract.
require(newOwner != address(0), "Ownable: new owner is the zero address");
NestedFactory is managing the parameter and it's always msg.sender.NestedFactory is managing the parameter and it's always msg.sender.NestedFactory is managing the parameter and it's always msg.sender.NestedFactory is managing the parameter and it's always msg.sender.renounceOwnership doesn't work.Already surfaced in the first audit : https://github.com/code-423n4/2021-11-nested-findings/issues/101
No reentrancy on owner functions...
_owner is used by the onlyTokenOwner modifier.
The rest is out of scope.
Not explaining why the comments are needed. Can be well explained in @notice or @dev.
No. onlyTokenOwner and onlyFactory
DeflationaryMockERC20.transferFrom is out of scope.FeeSplitter._addShares can't really happen for this function.OwnableFactoryHandler.removeFactory it's fine, will revert with "OFH: NOT_SUPPORTED".#1 - maximebrugel
2022-02-10T14:27:57Z
In conclusion, only confirmed for : "OwnableProxyDelegation.sol.initialize ownerAddr => (Confirmed)"
#2 - harleythedogC4
2022-03-03T02:18:13Z
My personal judgements:
#3 - harleythedogC4
2022-03-03T02:23:36Z
Now, here is the methodology I used for calculating a score for each QA report. I first assigned each submission to be either non-critical (1 point), very-low-critical (5 points) or low-critical (10 points), depending on how severe/useful the issue is. The score of a QA report is the sum of these points, divided by the maximum number of points achieved by a QA report. This maximum number was 26 points, achieved by #66.
The number of points achieved by this report is 5 points. Thus the final score of this QA report is (5/26)*100 = 19.
🌟 Selected for report: pauliax
Also found by: 0x1f8b, Dravee, GreyArt, Omik, ShippooorDAO, Tomio, bobi, cmichel, csanuragjain, defsec, gzeon, kenta, kenzo, m_smirnova2020, rfa, robee, sirhashalot, ye0lde
344.6384 USDC - $344.64
Title: State variables that could be set immutable Severity: GAS
In the following files there are state variables that could be set immutable to save gas.
operator in TestableOperatorCaller.sol resolver in MixinOperatorResolver.sol operatorStorage in ZeroExOperator.sol
Title: Unused state variables Severity: GAS
Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.
TestableMixingOperatorResolver.sol, addressesToCache
Title: Unused declared local variables Severity: GAS
Unused local variables are gas consuming, since the initial value assignment costs gas. And are a bad code practice. Removing those variables will decrease the gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.
TestableOperatorCaller.sol, performSwap, data
Title: Unnecessary array boundaries check when loading an array element twice Severity: GAS
There are places in the code (especially in for-each loops) that loads the same array element more than once. In such cases, only one array boundaries check should take place, and the rest are unnecessary. Therefore, this array element should be cached in a local variable and then be loaded again using this local variable, skipping the redundant second array boundaries check: NestedFactory.sol._processOutputOrders - double load of _batchedOrders[i] NestedFactory.sol._processInputOrders - double load of _batchedOrders[i]
Title: Caching array length can save gas Severity: GAS
Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:
for (uint256 i=0; i<array.length; i++) { ... }
to:
uint len = array.length for (uint256 i=0; i<len; i++) { ... } FeeSplitter.sol, _tokens, 148 MixinOperatorResolver.sol, requiredOperators, 55 NestedFactory.sol, operatorsCache, 103 NestedFactory.sol, orders._batchedOrders, 369 FeeSplitter.sol, shareholders, 261 FeeSplitter.sol, shareholdersCache, 280 FeeSplitter.sol, shareholders, 318 OperatorResolver.sol, names, 60 FeeSplitter.sol, _tokens, 165 OperatorResolver.sol, destinations, 75 MixinOperatorResolver.sol, requiredOperators, 36 NestedFactory.sol, _batchedOrders, 581
Title: Prefix increments are cheaper than postfix increments Severity: GAS
Prefix increments are cheaper than postfix increments.
Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change
There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)).
But increments perform overflow checks that are not necessary in this case.
These functions use not using prefix increments (++x) or not using the unchecked keyword:
change to prefix increment and unchecked: NestedFactory.sol, i, 153 change to prefix increment and unchecked: MixinOperatorResolver.sol, i, 55 change to prefix increment and unchecked: NestedFactory.sol, i, 103 change to prefix increment and unchecked: NestedFactory.sol, i, 327 change to prefix increment and unchecked: NestedRecords.sol, i, 196 change to prefix increment and unchecked: FeeSplitter.sol, i, 148 change to prefix increment and unchecked: FeeSplitter.sol, i, 318 change to prefix increment and unchecked: OperatorResolver.sol, i, 40 change to prefix increment and unchecked: OperatorResolver.sol, i, 60 change to prefix increment and unchecked: FeeSplitter.sol, i, 280 change to prefix increment and unchecked: FeeSplitter.sol, i, 165 change to prefix increment and unchecked: FeeSplitter.sol, i, 261 change to prefix increment and unchecked: MixinOperatorResolver.sol, i, 36 change to prefix increment and unchecked: NestedFactory.sol, i, 273 change to prefix increment and unchecked: NestedFactory.sol, i, 213 change to prefix increment and unchecked: NestedFactory.sol, i, 369 change to prefix increment and unchecked: NestedFactory.sol, i, 581 change to prefix increment and unchecked: OperatorResolver.sol, i, 75 change to prefix increment and unchecked: FeeSplitter.sol, i, 126 change to prefix increment and unchecked: NestedFactory.sol, i, 113 change to prefix increment and unchecked: NestedFactory.sol, i, 291
Title: Unnecessary index init Severity: GAS
In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:
MixinOperatorResolver.sol, 36 NestedFactory.sol, 153 OperatorResolver.sol, 75 NestedFactory.sol, 273 OperatorResolver.sol, 60 NestedFactory.sol, 213 FeeSplitter.sol, 318 MixinOperatorResolver.sol, 55 FeeSplitter.sol, 261 NestedFactory.sol, 291 NestedFactory.sol, 113 OperatorResolver.sol, 40 NestedFactory.sol, 369 NestedFactory.sol, 581 FeeSplitter.sol, 126 FeeSplitter.sol, 280 NestedFactory.sol, 103 FeeSplitter.sol, 165 NestedFactory.sol, 327 FeeSplitter.sol, 148 NestedRecords.sol, 196
Title: Internal functions to private Severity: GAS
The following functions could be set private to save gas and improve code quality:
MixinOperatorResolver.sol, callOperator NestedAsset.sol, _setTokenURI ExchangeHelpers.sol, setMaxAllowance ExchangeHelpers.sol, fillQuote MixinOperatorResolver.sol, requireAndGetAddress
Title: Public functions to external Severity: GAS
The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.
DeflationaryMockERC20.sol, transferFrom TestableMixingOperatorResolver.sol, resolverOperatorsRequired NestedRecords.sol, tokenHoldings NestedAsset.sol, originalOwner NestedRecords.sol, getAssetTokensLength NestedRecords.sol, freeHolding NestedAsset.sol, tokenURI OwnableProxyDelegation.sol, renounceOwnership OwnableProxyDelegation.sol, owner OwnableProxyDelegation.sol, transferOwnership
Title: Unnecessary payable Severity: GAS
The following functions are payable but msg.value isn't used - therefore the function payable state modifier isn't necessary. Payable functions are more gas expensive than others, and it's danger the users if they send ETH by mistake.
ZeroExOperator.sol, performSwap is payable but doesn't use msg.value FlatOperator.sol, transfer is payable but doesn't use msg.value
Title: Rearrange state variables Severity: GAS
You can change the order of the storage variables to decrease memory uses.
In OwnableProxyDelegation.sol,rearranging the storage fields can optimize to: 2 slots from: 3 slots. The new order of types (you choose the actual variables): 1. bytes32 2. address 3. bool
Title: Short the following require messages Severity: GAS
The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:
Solidity file: OwnableProxyDelegation.sol, In line 56, Require message length to shorten: 38, The message: Ownable: new owner is the zero address
Title: Unused imports Severity: GAS
In the following files there are contract imports that aren't used Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)
IFlatOperator.sol, line 3, import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; FlatOperator.sol, line 3, import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; INestedFactory.sol, line 4, import "../NestedReserve.sol";
Title: Unused inheritance Severity: GAS
Some of your contract inherent contracts but aren't use them at all. We recommend not to inherent those contracts. NestedAsset.sol; the inherited contracts OwnableFactoryHandler not used NestedReserve.sol; the inherited contracts OwnableFactoryHandler not used NestedRecords.sol; the inherited contracts OwnableFactoryHandler not used
Title: Use != 0 instead of > 0 Severity: GAS
Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)
NestedFactory.sol, 489: change 'balance > 0' to 'balance != 0'
Title: Unnecessary constructor Severity: GAS
The following constructors are empty. (A similar issue https://github.com/code-423n4/2021-11-fei-findings/issues/12)
TestableMixingOperatorResolver.sol.constructor NestedAsset.sol.constructor
Title: Unnecessary functions Severity: GAS
The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness. MixinOperatorResolver.sol, callOperator ExchangeHelpers.sol, fillQuote
Title: Unnecessary cast Severity: Gas
IERC20 NestedFactory.sol._transferInputTokens - unnecessary casting IERC20(_inputToken)
Title: Use calldata instead of memory Severity: GAS
Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.
NestedAsset.backfillTokenURI (_metadataURI) ExchangeHelpers.fillQuote (_swapCallData) NestedAsset._setTokenURI (_metadataURI) DeflationaryMockERC20.constructor (_name) NestedAsset.mintWithMetadata (_metadataURI) MockERC20.constructor (_symbol) DeflationaryMockERC20.constructor (_symbol) MockERC20.constructor (_name)
Title: Consider inline the following functions to save gas Severity: GAS
You can inline the following functions instead of writing a specific function to save gas. (see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.) FeeSplitter.sol, _computeShareCount, { return (_amount * _weight) / _totalWeights; }
Title: Inline one time use functions Severity: GAS
The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.
FeeSplitter.sol, _addShareholder MixinOperatorResolver.sol, requireAndGetAddress ExchangeHelpers.sol, setMaxAllowance
Title: Check if amount is not zero to save gas Severity: GAS
The following functions could skip other steps if the amount is 0. (A similar issue: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/82)
DeflationaryMockERC20.sol, transferFrom FeeSplitter.sol, sendFees
#0 - adrien-supizet
2022-02-15T09:41:37Z
Title: State variables that could be set immutable
Already done
Title: Unused state variables
invalid, mock file out of scope as stated in the readme
Title: Unused declared local variables
invalid, mock file out of scope as stated in the readme
Title: Unnecessary array boundaries check when loading an array element twice
Before: <img width="867" alt="image" src="https://user-images.githubusercontent.com/32484870/154038040-dd6d1d5f-fef6-40fa-b4ad-b01a7065a8fe.png">
After: <img width="869" alt="image" src="https://user-images.githubusercontent.com/32484870/154038469-cc53f79e-9461-4703-8625-31b45120f8c8.png">
Title: Caching array length can save gas
duplicate from the last report mentioned in the readme, it's already done where it was useful.
Title: Prefix increments are cheaper than postfix increments
duplicate from the last report mentioned in the readme and in #3 and we don't want to do this
Title: Unnecessary index init
invalid, this makes no difference in loops where the variables must be inited to 0 during the first iteration
Title: Internal functions to private
This wouldn't work.
Title: Public functions to external
Confirmed
Title: Unnecessary payable
This is untrue.
Title: Rearrange state variables
I see no difference whatsoever.
Title: Short the following require messages
Confirmed
Title: Unused imports
Invalid, this does not affect interfaces
Title: Unused inheritance
invalid, they are
Title: Use != 0 instead of > 0
The code mentioned does not exist
Title: Unnecessary constructor
invalid, It is necessary.
Title: Unnecessary functions
invalid, They are necessary.
Title: Unnecessary cast
confirmed, reserve.withdraw(IERC20(_inputToken), _inputTokenAmount);
Title: Use calldata instead of memory
confirmed for non-mock files
Title: Consider inline the following functions to save gas
confirme
Title: Inline one time use functions
True. Acknowledge or confirm?
Title: Check if amount is not zero to save gas
True but this adds an extra check for 99.999% of cases, so we don't want to do this
#1 - harleythedogC4
2022-03-13T05:31:15Z
My personal judgments:
operatorStorage is already set as immutable. Invalid.#2 - harleythedogC4
2022-03-13T06:20:54Z
Now, here is the methodology I used for calculating a score for each gas report. I first assigned each submission to be either small-optimization (1 point), medium-optimization (5 points) or large-optimization (10 points), depending on how useful the optimization is. The score of a gas report is the sum of these points, divided by the maximum number of points achieved by a gas report. This maximum number was 10 points, achieved by #67.
The number of points achieved by this report is 8 points. Thus the final score of this gas report is (8/10)*100 = 80.