code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

Claimer can steal all reward from user #167

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L607-L629 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1043-L1078 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L407-L476

Vulnerability details

When Claimer claim prize, claimer can set any fee and fee recipient address he/she want.

function claimPrizes(
    uint8 _tier,
    address[] calldata _winners,
    uint32[][] calldata _prizeIndices,
    uint96 _feePerClaim,
    address _feeRecipient
) external returns (uint256) {
    if (msg.sender != _claimer) revert CallerNotClaimer(msg.sender, _claimer);
    uint totalPrizes;
    for (uint w = 0; w < _winners.length; w++) {
        uint prizeIndicesLength = _prizeIndices[w].length;
        for (uint p = 0; p < prizeIndicesLength; p++) {
            totalPrizes += _claimPrize(
            _winners[w],
            _tier,
            _prizeIndices[w][p],
            _feePerClaim,
            _feeRecipient
        );
    }
}

There is only limitation of fee is prizeSize of the tier, which is not effect to attack

if (_fee > tierLiquidity.prizeSize) {
    revert FeeTooLarge(_fee, tierLiquidity.prizeSize);
}

And fee is directly transfered to feeRecipient address, which is controled by Receiver. Rest will be transfered to winner

if (_fee != 0) {
    emit IncreaseClaimRewards(_feeRecipient, _fee);
    claimerRewards[_feeRecipient] += _fee;
}

uint256 amount = tierLiquidity.prizeSize - _fee;
emit ClaimedPrize(
    msg.sender,
    _winner,
    _prizeRecipient,
    lastClosedDrawId,
    _tier,
    _prizeIndex,
    uint152(amount),
    _fee,
    _feeRecipient
);
_transfer(_prizeRecipient, amount);

So the attack scenario at here is: With each winner in the vault, malicious Claimer will calcualte prize depend on their tier, and set _feePerClaim equal to prize, along with _feeRecipient is address he/she control. Repeating that process, all reward will be stolen.

Impact

All reward can be stolen by malicious Claimer.

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

Remove Claimer role. create new mechanism that automatically transfer reward for user, along with constant fee

Assessed type

Other

Picodes commented 1 year ago

There is Claimer contract in-scope

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid