drawManager CAN BE SET TO A MALICIOUS ADDRESS

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L278-L281
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#L348
https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L335-L342

Vulnerability details

Impact

In the PrizePool.constructor the drawManager state variable is set as follows:

drawManager = params.drawManager;

But it accepts even if the params.drawManager == address(0). There is no input validation for the address(0) as well.

The PrizePool.setDrawManager function is used to set the drawManager if not already set. But the problem here is that there is no access control for this function and any one can call this. A malicious user can front run the valid drawManager assignment transaction and set a malicious address as the drawManager. The other issue is once the drawManager is set it can not be updated due to the following conditional check. Hence the contract will have to redeployed.

if (drawManager != address(0)) { //@audit-issue - can be front run by a malicious user
  revert DrawManagerAlreadySet();
}

Hence the following two functions controlled by the onlyDrawManager modifier will be vulnerable to attacks, since the drawManager is a malicious address now.

1. PrizePool.withdrawReserve can withdraw tokens from the reserve to any address given in the _to parameter. Hence this function will be vulnerable.

2. PrizePool.closeDraw() function can close the current open draw. This function is only callable by the drawManager. Hence a malicious user can get control of this function thus making this function vulnerable.

Proof of Concept

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

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

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

    emit DrawManagerSet(_drawManager);
  }

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

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

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

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

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

Tools Used

Manaul Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to check for address(0) in the constructor when drawManager is set or to implement access control in the PrizePool.setDrawManager function so that only the admin of the contract can call this function to set the drawManager if it is not set already.

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as duplicate of #356

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[asselstine]

Fixed in GenerationSoftware/pt-v5-prize-pool#31