Submitted by 0xdeadbeef0x, also found by apvlki, ulqiorra, immeas, fs0c, gjaldon, Aymen0909, SadBase, hansfriese, KIntern_NA, Krace, aashar, 0xNazgul, mrpathfindr, btk, mert_eren, Kumpa, waldenyan20, Kenshin, y1cunhui, KingNFT, 0Kage, rvi0x, 0xRobocop, eccentricexit, supernova, critical-or-high, peanuts, rvierdiiev, cccz, mgf15, and orion
If ERC777 tokens are used for rewards, the entire balance of rewards in the staking contract can get drained by an attacker.
ERC777 allow users to register a hook to notify them when tokens are transferred to them.
This hook can be used to reenter the contract and drain the rewards.
The issue is in the claimRewards in MultiRewardStaking.
The function does not follow the checks-effects-interactions pattern and therefore can be reentered when transferring tokens in the for loop. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L170-L187
As can be seen above, the clearing of the accruedRewards is done AFTER the transfer when it should be BEFORE the transfer.
Foundry POC
The POC demonstrates an end-to-end attack including a malicious hacker contract that steals the balance of the reward token.
Add the following file (drainRewards.t.sol) to the test directory: https://github.com/code-423n4/2023-01-popcorn/tree/main/test
To run the POC, execute the following command:
Expected results:
Foundry, VS Code
Follow the checks-effects-interactions pattern and clear out accruedRewards[user][_rewardTokens[i]] before transferring.
Additionally, it would be a good idea to add a ReentrancyGuard modifier to the function.
RedVeil (Popcorn) confirmed, but disagreed with severity 
