PoolTogether - Bobface's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L394-L402

Vulnerability details

Summary

Anyone can withdraw the yield fee from the Vault contract.

Impact

The Vault contract charges a fee on the accumulated yield, which is supposed to be sent to the yieldFeeRecipient_, which itself is set in the constructor and can only be updated with the owner-protected method setYieldFeeRecipient.

The mintYieldFee method withdraws the accumulated fee. However, it takes a parameter address recipient, to which it credits the fee, instead of yieldFeeRecipient_. Since this method is not protected, the yield fee can be withdrawn by anyone by supplying an arbitrary recipient.

Proof of Concept

Link to affected code

function mintYieldFee(uint256 _shares, address _recipient) external {
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

    _yieldFeeTotalSupply -= _shares;
    _mint(_recipient, _shares);

    emit MintYieldFee(msg.sender, _recipient, _shares);
}

Tools Used

None

Recommended Mitigation Steps

Remove the address _recipient parameter and always _mint to the yieldFeeRecipient_:

function mintYieldFee(uint256 _shares) external {
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

    _yieldFeeTotalSupply -= _shares;
    _mint(yieldFeeRecipient_, _shares);

    emit MintYieldFee(msg.sender, yieldFeeRecipient_, _shares);
}

Assessed type

Access Control