claimer can not be set to address zero as it's stated in code comments

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVaultFactory.sol#L80
https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L299
https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/abstract/Claimable.sol#L67

Vulnerability details

Impact

claimer can not be set to address zero if none is available yet as it's stated in code comments.

Proof of Concept

When we deploy a new PrizeVault there is a comment sections above the deploy func:

    /// @notice Deploy a new vault
    /// @dev Emits a `NewPrizeVault` event with the vault details.

@>    /// @dev `claimer` can be set to address zero if none is available yet.

    /// @dev The caller MUST approve this factory to spend underlying assets equal to `YIELD_BUFFER` so the yield
    /// buffer can be filled on deployment. This value is unrecoverable and is expected to be insignificant.
    /// @param _name Name of the ERC20 share minted by the vault
    /// @param _symbol Symbol of the ERC20 share minted by the vault
    /// @param _yieldVault Address of the ERC4626 vault in which assets are deposited to generate yield
    /// @param _prizePool Address of the PrizePool that computes prizes
    /// @param _claimer Address of the claimer
    /// @param _yieldFeeRecipient Address of the yield fee recipient

    
function deployVault(
      ...
    ) external returns (PrizeVault) {}

However it is not possible as there is a check in Claimable contract.

Vault Factory deployment goes to the Vault constructor:

    constructor(
        string memory name_,
        string memory symbol_,
        IERC4626 yieldVault_,
        PrizePool prizePool_,
        address claimer_,
        address yieldFeeRecipient_,
        uint32 yieldFeePercentage_,
        uint256 yieldBuffer_,
        address owner_
    ) TwabERC20(name_, symbol_, prizePool_.twabController()) Claimable(prizePool_, claimer_) Ownable(owner_) {}

where it forward a variables to the Claimable contract to initiate it:

    constructor(PrizePool prizePool_, address claimer_) {
        if (address(prizePool_) == address(0)) revert PrizePoolZeroAddress();
        prizePool = prizePool_;
        _setClaimer(claimer_);
    }

    function _setClaimer(address _claimer) internal {
@>       if (_claimer == address(0)) revert ClaimerZeroAddress();
        claimer = _claimer;
        emit ClaimerSet(_claimer);
    }

And _setClaimer() check for address(0) and revert the tx in case of that.

Tools Used

Manual review

Recommended Mitigation Steps

If it suppose to be allowed to be set as zero address initially, consider to set it directly in the constructor and make a _setClaimer() as public to let user or admin to set the claimer later.

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[raymondfam]

Code and comment mismatch. QA at best.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-c

[trmid]

Removed the misleading comment here: GenerationSoftware/pt-v5-vault#88