code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexRewards.sol#L373-#L417

Vulnerability details

Impact

Anyone can claim reward on behalf of others

Proof of Concept

Currently, function claim in PirexRewards does not check if caller is the user claiming reward and instead make user as a parameter. This makes anyone able to claim reward for others. There are many reasons why this should not happen. Imagine a user has a reward of 1000, then someone calls claim reward for him and when he call function getUserState, he will see his reward decrease significantly since it's claimed. Furthermore, there might be some users who want to keep his/her locked in system and claim it later.

Tools Used

Manually review

Recommended Mitigation Steps

I recommend that function claim should remove user parameter and claim reward for msg.sender instead, or at least check if msg.sender is the rewardRecipient if you want the recipients to be able to claim reward too.

Picodes commented 1 year ago

The Proof of Concept is not convincing.

I get the point that it could break some potential integrations, but one could argue that it is a valid design as long as no one can steal someone else rewards and that integrators have to deal with it.

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)