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: 2/27
Findings: 2
Award: $9,836.01
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: jayjonah8
8666.0852 USDC - $8,666.09
jayjonah8
In NestedFactory.sol going through the create() function which leads to the sendFeesWithRoyalties() => _addShares() function, Im not seeing any checks preventing someone from copying their own portfolio and receiving royalty shares for it and simply repeating the process over and over again.
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/FeeSplitter.sol#L152 https://github.com/code-423n4/2021-11-nested/blob/main/contracts/FeeSplitter.sol#L220
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/NestedAsset.sol#L69
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#L491
Manual code review
A require statement should be added not allowing users to copy their own portfolios.
#0 - maximebrugel
2021-11-16T15:30:51Z
Indeed, a user can copy his own portfolio to reduce the fees, however a require statement won't fix this issue...
This problem cannot be corrected but only mitigated, since the user can use two different wallets. Currently the front-end doesn't allow to duplicate a portfolio with the same address.
I don't consider this a "High Risk" since the assets are not really stolen. Maybe "Med Risk" ? This is by design an issue and we tolerate that users can do this (with multiple wallets).
#1 - alcueca
2021-12-03T16:09:27Z
I'm reading that the vulnerability actually lowers fees to zero for a dedicated attacker, since creating a arbitrarily large number of wallets and bypassing the frontend is easy. In theory leaking protocol value would be a severity 2, but since this is effectively disabling a core feature of the protocol (fees), the severity 3 is sustained.
jayjonah8
In NestedFactory.sol using Multicall.sol can be dangerous when it has a msg.value inside a loop since the msg.value doesn't update every iteration. This can lead to a user sending ETH one time and it being counted for every iteration. There is a msg.value in create() => _submitOrders => _transferInputTokens() which simply changes ETH for WETH. This _transferInputTokens() function is called alot in NestedFactory.sol by many functions. This can introduce serious bugs in the future as the protocol grows.
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L6
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Multicall.sol
https://samczsun.com/two-rights-might-make-a-wrong/
Manual code review
Remove the Open Zeppelin Multicall.sol functionality since it doesn't seem to be used and can introduce serious future bugs.
#0 - maximebrugel
2021-11-18T11:36:34Z
Duplicated : #226