maximebrugel
commented
2 years ago File: NestedRecords.sol
« Cache records[_nftId].reserve » (Acknowledged)
« Inclusive comparison » (Disputed)
Need to be strictly, if equal it will be exceed the limit after storing. But it could work with « + 1 ».
« function deleteAsset() »
« Unchecked block » (Acknowledged)
File: NestedFactory.sol
« removeOperator Unchecked block » (Acknowledged)
No unchecked blocks for admin functions.
« function destroy() Unchecked block (1) » (Disputed)
Don’t use unchecked when smart contract state is involved.
« function destroy() Unchecked block (2) » (Confirmed)
« function _submitInOrders() Unchecked block 1 & 2 » (Confirmed)
« function _submitOutOrders() Unchecked block 1 & 2 » (Confirmed)
« function _submitOutOrders() Unchecked block 3 » (Disputed)
Don’t use unchecked when smart contract state is involved.
« function _submitOutOrders() Unchecked block 4 » (Confirmed)
« function _safeSubmitOrder() Unchecked block » (Confirmed)
« function _transferToReserveAndStore() Unchecked block » (Disputed)
Don’t use unchecked when smart contract state is involved.
« function _transferInputTokens() Unchecked block » (Disputed)
Don’t use unchecked when smart contract state is involved.
« function _safeTransferWithFees() Unchecked block » (Confirmed)
File: FeeSplitter.sol
« function updateShareholder() : Cache `totalWeights » (Acknowledged)
« function sendFees() Unchecked block » (Disputed)
Don’t use unchecked when smart contract state is involved.
« function sendFeesWithRoyalties() Unchecked block » (Disputed)
Don’t use unchecked when smart contract state is involved.
« function sendFeesWithRoyalties() totalWeights » (Confirmed)
« function getAmountDue() Cache _tokenRecords.totalShares » (Confirmed)
« function _addShareholder() private function inlined » (Acknowledged)
Better readability in a private function and it’s an admin function.
File: ZeroExOperator.sol
« function performSwap() unchecked block 1 & 2 » (Disputed)
Don’t use unchecked when smart contract state is involved.
File: INestedFactory.sol
« Tightly pack struct BatchedInputOrders » (Disputed)
Increasing after testing.
« Tightly pack struct BatchedOutputOrders » (Disputed)
Increasing after testing.
« Only use 1 struct » (Disputed)
We don’t want an uint256 array for inputs.
General recommendations
« Amounts should be checked for 0 before calling a transfer » (Disputed)
No optimizations. [image:DFFADFE9-F216-4393-9AD7-7F5537B63B51-22545-0000C5BBF01A7052/8ED5AC0E-10BB-4671-ABE3-630BEAE0A364.png]
« ++I costs less gas compared to i++ & Increments can be unchecked » (Disputed)
Already surfaced => unchecked { ++I } is more gas efficient than I++ for loops · Issue #25 · code-423n4/2021-11-nested-findings · GitHub
« An array’s length should be cached to save gas in for-loops » (Disputed)
Already surfaced in first audit.
« Reduce the size of error messages (Long revert Strings) » (Duplicated)
« Use Custom Errors instead of Revert Strings to save Gas » ()
Already surfaced code-423n4/2021-11-nested-findings#14
harleythedogC4
Gas Report
Table of Contents:
records[_nftId].reservetotalWeightstotalWeights_tokenRecords.totalSharesBatchedInputOrdersBatchedOutputOrders++icosts less gas compared toi++Foreword
Storage-reading optimizations
Unchecking arithmetics operations that can't underflow/overflow
@audittagsFile: NestedRecords.sol
function store()
Cache
records[_nftId].reserveCaching this in memory can save around 1 SLOAD
Inclusive comparison
By definition,
maxHoldingsCountis theThe maximum number of holdings for an NFT record. Here, as an example, ifmaxHoldingsCount == 1andrecords[_nftId].tokens.length == 1, the function will revert. I believe this check should be inclusive (like thisrecords[_nftId].tokens.length <= maxHoldingsCount). This is both a Low-risk issue and a gas issue as<costs 3 more gas than<=due to the additionalISZEROopcode (even with the Optimizer)function deleteAsset()
Unchecked block
If
tokens.length == 1, all assets would be deleted. Iftokens.length == 0, line 90 would've thrown an error and trigger a revert. As it's impossible for line 95 to underflow, it should be wrapped inside anuncheckedblock.File: NestedFactory.sol
function removeOperator()
Unchecked block
Line 115 can't underflow due to
operatorsLength > 0(the for-loop wouldn't iterate otherwise). Therefore, line 115 should be wrapped inside anuncheckedblock.function destroy()
Unchecked block (1)
As
buyTokenInitialBalanceis<=to the final_buyToken.balanceOf(address(this)), line 222 can't underflow. Therefore, line 222 should be wrapped inside anuncheckedblock.Unchecked block (2)
As
amountBought -= amountFeesis equivalent toamountBought = amountBought - (amountBought / 100), the result can't underflow. Therefore, line 223 should be wrapped inside anuncheckedblock.function _submitInOrders()
Unchecked block (1)
Line 339 can't underflow due to the require statement line 337. Therefore, line 339 should be wrapped inside an
uncheckedblock.Unchecked block (2)
As
underSpentAmount = _inputTokenAmount - feesAmount - amountSpent(line 339):_inputTokenAmount >= underSpentAmount. Therefore, line 346 can't underflow and should be wrapped inside anuncheckedblock.function _submitOutOrders()
Unchecked block (1)
Line 387 can't underflow due to the require statement line 385. Therefore, line 387 should be wrapped inside an
uncheckedblock.Unchecked block (2)
As
underSpentAmount = _inputTokenAmount - amountSpent:_inputTokenAmount >= underSpentAmount. Therefore, line 392 can't underflow and should be wrapped inside anuncheckedblock.Unchecked block (3)
As the initial
_batchedOrders.outputToken.balanceOf(address(this))line 365 is<=to the final_batchedOrders.outputToken.balanceOf(address(this))line 395: line 395 can't underflow. Therefore, line 395 should be wrapped inside anuncheckedblock.Unchecked block (4)
As
amountBought - feesAmountis equivalent toamountBought - (amountBought / 100), the result can't underflow. Therefore, line 399 should be wrapped inside anuncheckedblock.function _safeSubmitOrder()
Unchecked block
Line 446 can't underflow due to the require statement line 445. Therefore, line 446 should be wrapped inside an
uncheckedblock.function _transferToReserveAndStore()
Unchecked block
As the initial
_token.balanceOf(reserveAddr)is<=to the final_token.balanceOf(reserveAddr): line 471 can't underflow. Therefore, line 471 should be wrapped inside anuncheckedblock.function _transferInputTokens()
Unchecked block
As the initial
_inputToken.balanceOf(address(this))is<=to the final_inputToken.balanceOf(address(this)): line 505 can't underflow. Therefore, it should be wrapped inside anuncheckedblock.function _safeTransferWithFees()
Unchecked block
As
_amount - feeAmountis equivalent to_amount - (_amount / 100), the result can't underflow. Therefore, line 574 should be wrapped inside anuncheckedblock.File: FeeSplitter.sol
function updateShareholder()
Cache
totalWeightsIt's possible to save around 1 SLOAD by caching
totalWeightsin memory, like this:function sendFees()
Unchecked block
As the initial
_token.balanceOf(address(this))is<=to the final_token.balanceOf(address(this)): line 184 can't underflow. Therefore, it should be wrapped inside anuncheckedblock.function sendFeesWithRoyalties()
Unchecked block
As the initial
_token.balanceOf(address(this))is<=to the final_token.balanceOf(address(this)): line 200 can't underflow. Therefore, it should be wrapped inside anuncheckedblock.Cache
totalWeightsCaching this in memory can save around 1 SLOAD
function getAmountDue()
Cache
_tokenRecords.totalSharesCaching this in memory can save around 1 SLOAD
function _addShareholder()
A private function used only once can get inlined
As this private function is only used once line 127 in function setShareholders(), it can get inlined to save some gas.
File: ZeroExOperator.sol
function performSwap()
Unchecked block (1)
As the initial
buyToken.balanceOf(address(this))is<=to the finalbuyToken.balanceOf(address(this)): line 34 can't underflow. Therefore, it should be wrapped inside anuncheckedblock.Unchecked block (2)
As the initial
sellToken.balanceOf(address(this))is<=to the finalsellToken.balanceOf(address(this)): line 35 can't underflow. Therefore, it should be wrapped inside anuncheckedblock.File: INestedFactory.sol
Storage
Tightly pack struct
BatchedInputOrdersstruct BatchedInputOrderscan be tightly packed to save 1 storage slot by changing the code from this:to this:
Tightly pack struct
BatchedOutputOrdersstruct BatchedOutputOrderscan be tightly packed to save 1 storage slot by changing the code from this:to this:
Only use 1 struct
In my opinion, the structs here are used in an unintended way: it's not up to a struct to carry the input/output concept here.
It's possible to use only 1 struct for the whole logic, as such:
And then declare input and output variables with this, like:
BatchedOrders[] _batchedInputOrdersorBatchedOrders[] _batchedOutputOrders.Same struct, different variables.
I suggest going from:
to
General recommendations
Variables
No need to explicitly initialize variables with default values
If a variable is not set/initialized, it is assumed to have the default value (
0foruint,falseforbool,address(0)for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.As an example:
for (uint256 i = 0; i < numIterations; ++i) {should be replaced withfor (uint256 i; i < numIterations; ++i) {Instances include:
I suggest removing explicit initializations for default values.
Comparisons
Amounts should be checked for 0 before calling a transfer
Checking non-zero transfer values can avoid an expensive external call and save gas.
Places I suggest adding a non-zero-value check:
For-Loops
++icosts less gas compared toi++++icosts less gas compared toi++for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)i++incrementsiand returns the initial value ofi. Which means:But
++ireturns the actual incremented value:In the first case, the compiler has to create a temporary variable (when used) for returning
1instead of2Instances include:
I suggest using
++iinstead ofi++to increment the value of an uint variable.Increments can be unchecked
In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.
ethereum/solidity#10695
Instances include:
The code would go from:
to:
The risk of overflow is inexistant for a
uint256here.An array's length should be cached to save gas in for-loops
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.
Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:
Errors
Reduce the size of error messages (Long revert Strings)
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.
Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
Revert strings > 32 bytes are here:
I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.
Use Custom Errors instead of Revert Strings to save Gas
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:
Custom errors are defined using the
errorstatement, which can be used inside and outside of contracts (including interfaces and libraries).Instances include:
I suggest replacing revert strings with custom errors.