PoolTogether - Jorgect's results

Lines of code

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

Vulnerability details

The prizePool contract is using a drawManager to call some importan function in the prizePool like withdrawReserve to withdraw the resever, closeDraw function in which it setting the winningNumber.

The drawManager is set in the contructor but also have a function to change it. This function it´s not implementing any access control or modifier to prevent that this function will be call it by a malicius user.

Impact

A malicius user can change the drawManager and withdraw the resever or call a closeDraw and set his own winning number

Proof of Concept

We can analyze the function and the comment:

file: Breadcrumbspt-v5-prize-pool/src/PrizePool.sol

 /// @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 {
        //@audit (high) evryone can set the drawmanager?
        if (drawManager != address(0)) {
            revert DrawManagerAlreadySet();
        }
        drawManager = _drawManager;

        emit DrawManagerSet(_drawManager);
    }

Link

For the comments "@dev Notice that this can be front-run: make sure to verify the drawManager after construction" we can assume that this function should be call it after the constructor in case that the the drawManager was not set. The problem is that this function can be call it without restrictions. permiting a malicius user becoming it self the drawManager and withdraw the resever:

file: Breadcrumbspt-v5-prize-pool/src/PrizePool.sol

function withdrawReserve(address _to, uint104 _amount) external onlyDrawManager {
    if (_amount > _reserve) {
      revert InsufficientReserve(_amount, _reserve);
    }
    _reserve -= _amount;
    _transfer(_to, _amount);
    emit WithdrawReserve(_to, _amount);
  }

Link

Or close the draw and set his own winning number:

 function closeDraw(uint256 winningRandomNumber_) external onlyDrawManager returns (uint16) {
   ...
}

Link

Tools Used

manual

Recommended Mitigation Steps

The contract can implement a initializer modifier in the setDrawManager function:

uint256 public initializing = 0;
modifier initializer(){
    if (initializing == 1){
        revert;
    }
    initializing=1;
    
}

 function setDrawManager(address _drawManager) external initializer{
        ...
    }

The contract can add onlyDrawManager in the setDrawManager function:

function setDrawManager(address _drawManager) external onlyDrawManager{
        ...
    }

Assessed type

Access Control