code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardEscrow.sol#L38 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardEscrow.sol#L42

Vulnerability details

Impact

MultiRewardEscrow functions getEscrowIdsByUser() and getEscrowIdsByUserAndToken() are the only way for a user to find their escrow ids as there are no events with an escrow id emitted. An attacker can call the MultiRewardStaking claimRewards() function multiple times for a victim's account and cause problems with gas exhaustion on the victim's side, so the victim would have no way to know what escrow ids are belong to them.

The attack is realistic in a scenario where MultiRewardStaking contract has many different reward tokens added: at least 10 or 100.

Proof of Concept

There are two functions:

function getEscrowIdsByUser(address account) external view returns (bytes32[] memory) {
  return userEscrowIds[account];
}

function getEscrowIdsByUserAndToken(address account, IERC20 token) external view returns (bytes32[] memory) {
  return userEscrowIdsByToken[account][token];
}

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardEscrow.sol#L38

These two functions are currently the only way for a user to find out which escrow ids belong to them, as no events with an escrow identifier are thrown when it is created.

An escrow account is created for a correctly added MultiRewardStaking reward token each time the user calls MultiRewardStaking.claimRewards(): this function calls _lockToken(), which creates a new escrow vault.

_lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);

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

If 100 different reward tokens are added to the contract, each of which is added to MultiRewardEscrow, it is enough for an attacker to call the publicly available claimRewards() function about 100 times on different blocks and specify the victim's account as a parameter, so that the victim's userEscrowIds variable accumulates 100 * 100 = 10_000 records. This can cause problems with gas exhaustion for the victim.

Tools Used

Manual analysis

Recommended Mitigation Steps

Emit every escrow ID in an event

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)