code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

`PrizePool.sol` is prone to `DOS` if `drawManager` role is not set in constructor #297

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L299

Vulnerability details

Impact

setDrawManager offers Attacker to frontrun the tx which can lock drawManager role. User will have to do costly redeployment which again prone to frontrun attack if he doesn't specify drawManager role in constructor and leads to DOS .

scenarios

  1. if drawManager has not been set in constructor [DOS] this will lead to DOS as for every deployment Attacker can frontrun the setDrawManager. This will lock the drawManager role and so prizePool

  2. As in constructor there is no address check for address so invalid address will cost redeployment

Proof of Concept

see Impact

File: src/PrizePool.sol

258 constructor(
    ConstructorParams memory params
  )
    TieredLiquidityDistributor(
      params.numberOfTiers,
      params.tierShares,
      params.canaryShares,
      params.reserveShares
    )
  {
    if (unwrap(params.smoothing) >= unwrap(UNIT)) {
      revert SmoothingGTEOne(unwrap(params.smoothing));
    }
    prizeToken = params.prizeToken;
    twabController = params.twabController;
    smoothing = params.smoothing;
    claimExpansionThreshold = params.claimExpansionThreshold;
    drawPeriodSeconds = params.drawPeriodSeconds;
    _lastClosedDrawStartedAt = params.firstDrawStartsAt;

    drawManager = params.drawManager;
    if (params.drawManager != address(0)) {
      emit DrawManagerSet(params.drawManager);
    }
282  }

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

306    emit DrawManagerSet(_drawManager);
  }

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L258C2-L283C1

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L299C1-L307C1

Tools Used

Manual

Recommended Mitigation Steps

set the drawManager role to deployer(msg.sender) in constructor and then add a option to change setdrawManager where deployer can set drawManager to any address

Assessed type

Access Control

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #356

c4-judge commented 1 year ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory