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: 19/27
Findings: 2
Award: $331.34
🌟 Selected for report: 1
🚀 Solo Findings: 0
loop
Considering _token can be any ERC20 token, safeTransfer should be used rather than transfer, especially since it is already imported and used in the rest of the contract.
Impact is very low considering unlockTokens will only ever need to be called if a user made a mistake by transferring tokens to the contract.
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L272
#0 - maximebrugel
2021-11-16T14:33:39Z
Duplicated : #78
🌟 Selected for report: xYrYuYx
Also found by: PierrickGT, loop, palina, pauliax
loop
reserve is set after contract initialization by invoking the setReserve function. Some functions use the reserve address by making an external call to it or make us of address(reserve). While some of these functions require nestedRecords.getAssetReserve(_nftId) == address(reserve), which should make sure the reserve address is not address(0), the withdraw function has no requirement for reserve to be set.
If reserve isn't set, either by forgetting/failing to call setReserve or accidentally setting it to the zero address, the functions making a call to reserve will revert.
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L257
Check if reserve is set correctly in withdraw. Also make sure reserve isn't accidentally set to the zero address by adding a zero address check to setReserve for the _reserve parameter.
#0 - adrien-supizet
2021-11-19T09:43:33Z
This could happen, there is no incentive for anyone to do that. On our side we need to make sure everything is set correctly when we deploy.
The only solution would be the change the deployment flow and deploy the Reserve from the Factory constructor. We do not want to make such a change because it will not be reviewed by enough people before we deploy.
#1 - adrien-supizet
2021-11-22T15:30:33Z
duplicate #83
#2 - alcueca
2021-12-03T10:42:14Z
Taking #108 as main
loop
_calculateFees only makes use of _amount to calculate fees, the _user parameter can be removed.
Since _calculateFees is only called internally it does add to gas costs, an unused parameter adds a small amount of unnecessary gas.
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L557-L559
Remix
#0 - maximebrugel
2021-11-17T15:11:29Z
Duplicated : #167
#1 - alcueca
2021-12-03T10:16:20Z
Also comments are wrong (#194)
🌟 Selected for report: hyh
Also found by: MaCree, elprofesor, fatima_naz, gpersoon, gzeon, loop, palina, pauliax, pmerkleplant, xYrYuYx, ye0lde
loop
When calling removeOperator with an operator which isn't in the operators array the while loop condition operators[i] != operator will never be met. As a result the loop just keeps going until it reaches the gas limit and then reverts.
Impact isn't too big considering it's an onlyOwner function unlikely to be called with the wrong operator.
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L79-L86
Remix
#0 - maximebrugel
2021-11-16T11:27:51Z
Duplicated : #58
#1 - alcueca
2021-12-03T10:59:06Z
Not a duplicate of #58, just affecting the same code.
#2 - alcueca
2021-12-03T11:04:49Z
Taking #220 as the main and this as a duplicate
47.7068 USDC - $47.71
loop
The removeFactory has an unnecessary ==true comparison in its require statement. Since require already checks if the condition is true, there is no need for it to be compared.
Removing == true saves a tiny amount of gas if removeFactory is called.
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedAsset.sol#L142