code-423n4 / 2024-03-pooltogether-findings

5 stars 4 forks source link

`claimYieldFeeShares()` sets `yieldFeeBalance = 0` instead of `yieldFeeBalance -= _shares` #328

Closed c4-bot-6 closed 5 months ago

c4-bot-6 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L617

Vulnerability details

Impact

Yield fees are lost.

Proof of Concept

Yield fees are intermediately accounted for in yieldFeeBalance, later to be minted as normal shares by calling claimYieldFeeShares():

function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient {
    if (_shares == 0) revert MintZeroShares();

    uint256 _yieldFeeBalance = yieldFeeBalance;
    if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);

    yieldFeeBalance -= _yieldFeeBalance;

    _mint(msg.sender, _shares);

    emit ClaimYieldFeeShares(msg.sender, _shares);
}

claimYieldFeeShares(_shares) mints _shares shares but always sets yieldFeeBalance = 0 instead of yieldFeeBalance -= _shares, even when _shares < yieldFeeBalance. A subsequent call to claim the remaining yieldFeeBalance - _shares fees would therefore revert, i.e. the remaining yield fees are lost.

Recommended Mitigation Steps

- yieldFeeBalance -= _yieldFeeBalance;
+ yieldFeeBalance -= _shares;

Assessed type

Math

c4-pre-sort commented 5 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #10

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #59

c4-judge commented 5 months ago

hansfriese marked the issue as satisfactory