Platform: Code4rena
Start Date: 11/11/2021
Pot Size: $50,000 USDC
Total HM: 9
Participants: 27
Period: 7 days
Judge: alcueca
Total Solo HM: 5
Id: 53
League: ETH
Rank: 14/27
Findings: 2
Award: $1,023.28
🌟 Selected for report: 3
🚀 Solo Findings: 0
🌟 Selected for report: ye0lde
866.6085 USDC - $866.61
ye0lde
The functions below fail to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior.
names, destinations https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/OperatorResolver.sol#L27-L39
_inputTokenAmounts, orders https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L321-L337
Visual Studio Code, Remix
Add input validation to check that the length of both arrays match.
#0 - maximebrugel
2021-11-19T11:15:50Z
Confirm for the first one (names & destinations).
But disagree for the second one, in fact the check is done in the parent functions :
require(_sellTokensAmount.length == _orders.length, "NestedFactory::sellTokensToNft: Input lengths must match"); (...) (uint256 feesAmount, ) = _submitOutOrders(_nftId, _buyToken, _sellTokensAmount, _orders, true, true);
🌟 Selected for report: hyh
Also found by: MaCree, elprofesor, fatima_naz, gpersoon, gzeon, loop, palina, pauliax, pmerkleplant, xYrYuYx, ye0lde
ye0lde
This function does not exit the while loop gracefully when the operator being removed does not exist. There is a "require" statement after the while loop meant to handle this case but it is never reached.
The function is here: https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L79-L86
Visual Studio Code, Remix
I suggest this refactoring:
function removeOperator(bytes32 operator) public { uint256 foundIndex; bool found = false; for (uint256 i; !found && i < operators.length; i++) { if (operators[i] == operator) { found = true; foundIndex = i; } } require(found, "NestedFactory::removeOperator: Can't remove non-existent operator"); delete operators[foundIndex]; }
#0 - maximebrugel
2021-11-17T13:03:44Z
Duplicated : #58
#1 - alcueca
2021-12-03T11:01:43Z
Duplicate of #59
#2 - alcueca
2021-12-03T11:03:50Z
Actually, taking #220
47.7068 USDC - $47.71
ye0lde
Using existing local variables instead of reading state variables will save gas by converting SLOADs to MLOADs.
_nstReserve can be used here: https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedBuybacker.sol#L69
_feeSplitter can be used here: https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedBuybacker.sol#L76
_burnPercentage can be used here: https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedBuybacker.sol#L84
_maxHoldingsCount can be used here: https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedRecords.sol#L173
Visual Studio Code, Remix
See Proof of Concept
#0 - adrien-supizet
2021-11-19T11:12:11Z
We believed it would be true, after testing it actually shows no difference.
#1 - alcueca
2021-12-03T10:32:02Z
It should be true, even after the London fork. Reading from memory costs a minimum of 100 gas.
10.4335 USDC - $10.43
ye0lde
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
I can see that the sponsor is committed to verbose revert strings as almost every revert string is > 32 bytes but I wanted to at least mention this issue.
Almost every revert string in the project is > 32 bytes.
Visual Studio Code
Consider shortening the revert strings to fit in 32 bytes or using custom errors (v0.8.4 or greater) in the future.
#0 - adrien-supizet
2021-11-18T12:42:28Z
Duplicate #14
🌟 Selected for report: ye0lde
Also found by: PierrickGT, WatchPug, hack3r-0m, pmerkleplant
ye0lde
Removing the unused local variables improves code clarity.
Suggest changing to (uint256[] memory amounts,)
https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L379
https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L410
Visual Studio Code, Remix
Remove the unused variables - See POC
#0 - maximebrugel
2021-11-30T16:18:34Z
Duplicated #67 & #66
#1 - alcueca
2021-12-03T09:58:30Z
Taking this as main.
ye0lde
The comments for this function state that fees are calculated for a specific user. But the "user" parameter is not used and there is just a generic fee calculation.
The "specific user" comments are here: https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L553-L554
The unused "_user" parameter and generic fee calculation are here: https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L557-L558
Literal instead of constant/immutable for 1% used here: https://github.com/code-423n4/2021-11-nested/blob/f646002b692ca5fa3631acfff87dda897541cf41/contracts/NestedFactory.sol#L558
Visual Studio Code, Remix
Correct the comments and remove the "_user" parameter if it is not needed.
Or if "_user" will be used in the future, provide comments about why and when it will be in use.
Consider using a constant for 1%.
#0 - adrien-supizet
2021-11-19T17:07:11Z
duplicate #167