Closed code423n4 closed 2 years ago
not an exploit
I understand the willingness to avoid code duplication, however a STATICCALL would require more gas Inlining the same private constant throughout the codebase makes sense to me
Would be non-critical / gas at best, however I don't see any advantage in applying this improvement
Setting as invalid
Handle
pauliax
Vulnerability details
Impact
All three contracts (Auction, Basket, Factory) separately declare a constant variable named BASE with a value of 1e18. I am not sure if there is a reason for these values to change independently but in my opinion, it would be better to extract it to a common Constants contract that you can import into each of these contracts. The potential problem that I see is when Basket contract handles fees, it is supposed to mint some tokens to the publisher and what is left to the factory owner: _mint(publisher, fee (BASE - factory.ownerSplit()) / BASE); _mint(Ownable(address(factory)).owner(), fee factory.ownerSplit() / BASE); However, if different BASE values would be used in the Basket and Factory contracts, this code could mint unexpected amounts or even revert, e.g. if the factory starts using a higher value for a BASE ownerSplit then the subtraction may underflow and fail.
Recommended Mitigation Steps
My suggestion is to remove the duplication of the BASE variable, store it in a separate Constants contract, and re-use it where necessary.