code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L256-L282 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L296-L306

Vulnerability details

Impact

The DrawManager role in PrizePool is pivotal to correct operation, and it can be stolen. Sabotaging the deployment of PrizePool and all other contract deployed that hold an immutable reference to the singleton PrizePool.

Proof of Concept

On creation of the PrizePool, the constructor parameter of DrawManger is left as address(0), a valid creation configuration.

Then call setDrawManager with an unusable address e.g. address(1) https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L296-L306

  function setDrawManager(address _drawManager) external {
    if (drawManager != address(0)) {
      revert DrawManagerAlreadySet();
    }
    drawManager = _drawManager;

    emit DrawManagerSet(_drawManager);
  }

When setDrawManager is called with the _drawManager address, as the current drawManager is address(0), the given dead address is set as the draw manager and the PrizePool is now broken, as without the draw manager there can be no draws.

The draw manager is responsible for closing draws (which also starts new draws).

Tools Used

Manual review.

Recommended Mitigation Steps

Force the drawManager to be set on creation, make it immutable and delete setDrawManager().

If the address of PrizePool is required by other contracts whose addresses are part of the constructor params, consider using CREATE2 to generate the address for PrizePool before deployment, which would allow decoupling of contract deployments.

Assessed type

Access Control

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #356