code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

After frontrunning the setDrawManager functionAttacker can become drawManager of thePrizePool contract if once set then it can not be update by any address so contract and it's reserves funds become vulnerable. After becoming drawManager attacker can call withdrawReserve and withdraw all the reserves to his account. He can also call closeDraw and close the current open draw and open the next one. He updates the number of tiers, the winning random number and the prize pool reserve.

Proof of Concept

When contract PrizePool.sol is deployed, there is no address(0) check in constructor for drawManager so after deploying the contract if address(0) is passed by deployer by mistake. Now deployer will check the event DrawManagerSet(params.drawManager) , if not emitted he will take action to set DrawManager by calling the function setDrawManager but here Attacker can frontrun the setDrawManager function and set the drawManager before deployer. Now no one can change the drawManager. Attacker can use this for their advantage until tha contract is redeployed.

Vulnerable Code : src/PrizePool.sol#L258-L307

constructor and setDrawManager function.

 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;

278:    drawManager = params.drawManager;
279:    if (params.drawManager != address(0)) {
280:      emit DrawManagerSet(params.drawManager);
    }
  }

  /* ============ Modifiers ============ */

  /// @notice Modifier that throws if sender is not the draw manager.
  modifier onlyDrawManager() {
    if (msg.sender != drawManager) {
      revert CallerNotDrawManager(msg.sender, drawManager);
    }
    _;
  }

  /* ============ External Write Functions ============ */

  /// @notice Allows a caller to set the DrawManager if not already set.
  /// @dev Notice that this can be front-run: make sure to verify the drawManager after construction
    /// @param _drawManager The draw manager
   function setDrawManager(address _drawManager) external {
300:   if (drawManager != address(0)) {
301:      revert DrawManagerAlreadySet();
    }
303:    drawManager = _drawManager;

    emit DrawManagerSet(_drawManager);
 307: }

Tools Used

Manual Review

Recommended Mitigation Steps

First check for address(0) in constructor before assigning address passed to drawManager. If address(0) passed by mistake then deployment will fail and need to re-deploy the contract.To save gas fail as early as possible so put address(0) checks in the beginning of constructor.
After this there will be no need of setDrawManager function but if deployer want to update drawManager again then add proper access control to setDrawManger function to avoid this attack.Add owner variable so only deployer can set again not everyone. If deployer does not want to set again the remove setDrawManager function and add only address(0) check in constructor for drawManager before assigning.

Assessed type

Access Control

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #356