Closed code423n4 closed 2 years ago
this is intended behavior
Beside the "intended behaviour"part of the argument I don't see anything wrong with separating the two functions, it just means a EOA can create a basket and someone else can pay for it's creation
In the lack of any actual poc I'm setting this to invalid
Handle
0xRajeev
Vulnerability details
Impact
It is not clear why proposeBasketLicense() and createBasket() are two different functions but if the idea is to force them into two separate transactions then we can add a timelock between them to force creation to happen only after a certain delay has been enforced to monitor fake/duplicate baskets and alert upcoming creations of such baskets.
Given that creation can be done by a non-proposer/non-publisher, it may allow griefing attackers to front-run and fund baskets (not sure of the motivation) or worse if proposers assume certain IDs will be allotted to their baskets and issue createBasket before propose returns, they may fund a different basket than intended.
Proof of Concept
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Factory.sol#L65-L91
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Factory.sol#L93-L116
Tools Used
Manual Analysis
Recommended Mitigation Steps
Recommend adding a function to atomically propose+create in same Tx, or enforce a timelock between the propose and create functions or clearly specify the requirement for the need of two separate functions.