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 validdrawManager 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.
PrizePool.withdrawReserve can withdraw tokens from the reserve to any address given in the _to parameter. Hence this function will be vulnerable.
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.
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.
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
thedrawManager
state variable is set as follows: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 thedrawManager
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 thevalid
drawManager
assignment transaction and set a malicious address as thedrawManager
. The other issue is once thedrawManager
is set it can not be updated due to the following conditional check. Hence the contract will have to redeployed.Hence the following two functions controlled by the
onlyDrawManager
modifier will be vulnerable to attacks, since thedrawManager
is a malicious address now.PrizePool.withdrawReserve can withdraw tokens from the
reserve
to any address given in the_to
parameter. Hence this function will be vulnerable.PrizePool.closeDraw()
function can close the current open draw. This function is only callable by thedrawManager
. Hence a malicious user can get control of this function thus making this function vulnerable.Proof of Concept
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
Tools Used
Manaul Review and VSCode
Recommended Mitigation Steps
Hence it is recommended to check for
address(0)
in the constructor whendrawManager
is set or to implementaccess control
in thePrizePool.setDrawManager
function so that only the admin of the contract can call this function to set thedrawManager
if it is not set already.Assessed type
Invalid Validation