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

5 stars 4 forks source link

function claimYieldFeeShares doesnt update yieldFeeBalance accurately #264

Closed c4-bot-3 closed 7 months ago

c4-bot-3 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L611-L622

Vulnerability details

Impact

YieldFeeRecipient could lose part of his yield fee

Proof of Concept

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L611-L622

    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);
    }

Lets say yieldFeeBalance is 100 and the yield fee recipient wants to claim 50. The yieldFeeBalance becomes 0 but the msg.sender gets minted only 50. The other 50 shares will be lost since the yield fee recipient won't be able to claim them.

Tools Used

Manual Review

Recommended Mitigation Steps

Change yieldFeeBalance -= _yieldFeeBalance; to yieldFeeBalance -= _shares;

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

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

        yieldFeeBalance -= _shares;

        _mint(msg.sender, _shares);

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

Assessed type

Math

c4-pre-sort commented 7 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 7 months ago

raymondfam marked the issue as duplicate of #10

c4-pre-sort commented 7 months ago

raymondfam marked the issue as duplicate of #59

c4-judge commented 7 months ago

hansfriese changed the severity to 3 (High Risk)

c4-judge commented 7 months ago

hansfriese marked the issue as satisfactory