code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L170 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L371 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L121

Vulnerability details

Impact

If admin will add more than 255 tokens in rewardToken[], there would'nt any way to withdraw tokens or claim rewards

Proof of Concept

for example: all users deposit their money.After some time admin's add more tokensa, length of array with Token grow's to 256. Maliscious or just don't know about this bug. Function withdraw/claimRewards/etc. would revert after 255's token in any case. all function with accrueReward modifier become disabled. Because when casting i to uint8 in i < array.lentgh it will simply overflow. There is no point in using uint8, and don't save any gas.

Tools Used

Foundry as example of overflowing

Recommended Mitigation Steps

set i to uint256 to uint8 in all cycles that have this logic
add function deleteToken, but it's harder to count user's money

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Insufficient quality

code-423n4 / 2023-01-popcorn-findings