code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L270-L274 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L281-L298 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L373-L417

Vulnerability details

Impact

The functions globalAccrue, userAccrue and claim in PirexRewards never checks the inputted token address. This causes the contract to emit events with any random token. This won't lead to loss of funds since the rewardTokens array of a random producedr token is empty, however this will cause the contract to emit events which can be misleading for the UI/API. The contract even emits a claim event with random token addresses.

Proof of Concept

function testEvent() public {
        _setupRewardsAndTestAccounts(1);

        // Random coin
        newERC stc = new newERC();
        stc.mint(testAccounts[0], 10 ether);

        pirexRewards.globalAccrue(stc);
        pirexRewards.userAccrue(stc, address(testAccounts[0]));

        skip(1 days);

        pirexRewards.globalAccrue(stc);

        vm.expectEmit(true, true, false, false, address(pirexRewards));

        emit Claim(stc, address(testAccounts[0]));
        pirexRewards.claim(stc, testAccounts[0]);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Check if the producerToken is allowed before emitting claim event

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

Picodes commented 1 year ago

Downgrading to QA because this do not lead to funds loss or impact the functionality of the protocol. Note that this would be easily manageable by anyone using the events off chain.