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: 7/27
Findings: 3
Award: $2,227.17
🌟 Selected for report: 4
🚀 Solo Findings: 0
1169.9215 USDC - $1,169.92
hyh
Contract holdings can be emptied as malicious user will do deposit/withdraw to extract value. This is possible because after _transferInputTokens system uses contract balance for user's operations, assuming that equivalent value was transferred.
msg.value persist over calls, so passing 'Order[] calldata _orders' holding multiple ETH deposits will use the same msg.value in each of them, resulting in multiple deposits, that sums up to much bigger accounted value than actually deposited value, up to contract's ETH holdings.
create / addTokens -> _submitInOrders -> _transferInputTokens https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L103 https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L119
sellTokensToWallet -> _submitOutOrders -> _transferInputTokens https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L172
sellTokensToNft -> _submitOutOrders -> _transferInputTokens https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L152
_transferInputTokens uses msg.value: https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L462
Controlling ETH to be only once in orders will not help, as NestedFactory inherits from Multicall, which multicall(bytes[] calldata data) function allows same reusage of msg.value, which will persist over calls.
So, it is recommended to treat ETH exclusively, not allowing ETH operations to be batched at all.
#0 - adrien-supizet
2021-11-18T11:37:09Z
Multicall is not currently used, and the funds exposed would be the NestedFactory's which should hold no funds.
To avoid future bugs, we're going to remove the multicall library, but we don't think this is a high severity issue.
#1 - alcueca
2021-12-03T09:49:22Z
Downgrading severity to 2 because the NestedFactory is not expected to hold funds, and therefore there is no risk of a loss. You can't deposit the same Ether twice in the WETH contract.
Also keeping this as the main over #13.
🌟 Selected for report: hyh
866.6085 USDC - $866.61
hyh
NFT token operations will fail if wrong reserve is used.
NestedFactory reserve is used in addtokens and withdraw function for a given NFT, but the NFT to reserve contract correspondence isn't checked.
addtokens: https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L119
withdraw: https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L241
Add the require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "...") check in the beginning of the functions.
🌟 Selected for report: hyh
Also found by: MaCree, elprofesor, fatima_naz, gpersoon, gzeon, loop, palina, pauliax, pmerkleplant, xYrYuYx, ye0lde
hyh
Current implementation throws if first operator is to be deleted, i.e. operators[0] == operator, and doesn't throw when operator is not found, i.e. there is no i such that operators[i] == operator. This way an expected logic of throwing whenever operator isn't found in current list and deleting the one found otherwise doesn't take place.
This way
operators[0] cannot be deletedoperators list, an array bounds check violation will happenNestedFactory.removeOperator code: https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L79
Function code needs to be updated, for example:
Now:
uint256 i = 0; while (operators[i] != operator) { i++; } require(i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator"); delete operators[i];
To be:
for (uint256 i = 0; i < operators.length; i++) { if (operators[i] == operator) { break; } } require(i < operators.length, "NestedFactory::removeOperator: Can't remove non-existent operator"); delete operators[i];
#0 - maximebrugel
2021-11-18T07:17:09Z
Duplicated : #58
#1 - alcueca
2021-12-03T11:03:15Z
Taking this as the main for the whole lot.
🌟 Selected for report: hyh
106.015 USDC - $106.02
hyh
Whenever condition of the _handleUnderSpending function fails function call gas costs are wasted. The cost of checking the condition is paid anyway, while when it doesn't hold the function call costs are avoidable.
_handleUnderSpending checks for _amountToSpent - _amountSpent > 0.
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L481
When the check condition is false _handleUnderSpending shouldn't be called and this way the check with corresponding variables to be placed in caller functions:
_submitInOrders https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L306
_safeSubmitOrder https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L415
hyh
As the _calculateFees function operates only its agruments it can be pure.
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L557
#0 - maximebrugel
2021-11-18T08:42:01Z
Duplicated : #167
#1 - alcueca
2021-12-03T10:14:36Z
Also comments are wrong (#194)