NO TIMELOCK ON setExitFees() CAN LEAD TO USERS LOSING FUNDS IN NESTEDFACTORY
In NestedFactory.sol, there is no timelock on setExitFees(). This is the fee that is applied upon calling destroy(), and determines how much the FeeSplitter receives in fee VS how much the user receives (in ERC20 token). But:
there is no maximum value limiting what fee can be set to
there is no timelock, ie exitFees is set to its new value when setExitFees() is created.
A malicious owner could effectively wait for enough users to create portfolios, then set a very high fee, which would result in every user calling destroy() transferring the ERC20 token amount from the user to the feeSplitter, resulting in the user effectively losing their NFT for nothing.
Impact
Medium
Proof Of Concept
The malicious owner calls setExitFees(10000), setting exitFees as 100%.
A user calls destroy() to destroy one of their NFT.
amountFees = (amountBought * exitFees) / 10000 = amountBought, the fee is 100% of the amount;
the internal calls transfer amountBought tokens to the feeSplitter, and nothing to _msgSender()
Lines of code
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L169 https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L264 https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L269
Vulnerability details
NO TIMELOCK ON
setExitFees()
CAN LEAD TO USERS LOSING FUNDS IN NESTEDFACTORYIn
NestedFactory.sol
, there is no timelock onsetExitFees()
. This is the fee that is applied upon callingdestroy()
, and determines how much theFeeSplitter
receives in fee VS how much the user receives (in ERC20 token). But:exitFees
is set to its new value whensetExitFees()
is created.A malicious owner could effectively wait for enough users to create portfolios, then set a very high fee, which would result in every user calling
destroy()
transferring the ERC20 token amount from the user to thefeeSplitter
, resulting in the user effectively losing their NFT for nothing.Impact
Medium
Proof Of Concept
The malicious owner calls
setExitFees(10000)
, settingexitFees
as 100%.destroy()
to destroy one of their NFT.amountFees = (amountBought * exitFees) / 10000 = amountBought
, the fee is 100% of the amount;amountBought
tokens to thefeeSplitter
, and nothing to_msgSender()
Tools Used
Manual Analysis
Recommended Mitigation Steps
Two things can be done:
setExitFees()
setExitFees()
.