An attacker can install himself as a DrawManager and steal funds in the withdrawReserve() functions in PrizePool.sol

[code423n4]

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

Vulnerability details

Impact

The drawManager parameter can be set in the constructor (https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L278). However, if drawManager = 0 is passed to the constructor, it can be set later in setDrawManager() (https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L299- L306). The drawManager parameter in setDrawManager() is only updated once.

If you don't set drawManager in the constructor, an attacker can get ahead and set any address in setDrawManager(). The administrator will not be able to change it in the future.

The drawManager parameter is used in modifier onlyDrawManager() { if (msg.sender != drawManager) { revert CallerNotDrawManager(msg.sender, drawManager); } _; } at the time of checking access to the withdrawReserve() and closeDraw() functions.

Suppose someone managed to deposit funds into the prize pool via increaseReserve(). This could result in the funds in withdrawReserve() being stolen by an attacker (https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L335).

Tools Used

Manual review

Recommended Mitigation Steps

Only set the value of drawManager in the constructor. Or set the onlyOwner check for setDrawManager().

Assessed type

Access Control

[c4-judge]

Picodes marked the issue as primary issue

[asselstine]

This is easily checked after deployment; it's a classic chicken-or-the-egg problem where both contracts need to reference eachother.

[c4-sponsor]

asselstine marked the issue as disagree with severity

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked issue #431 as primary and marked this issue as a duplicate of 431