code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L182 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L457 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L526

Vulnerability details

Impact

The vulnerability in the MultiRewardStaking and VaultController contracts lies in the usage of the transfer and transferFrom functions, which does not provide the safety checks for the transfer of tokens, especially since the reward token can have arbitrary implementation. If the recipient contract does not have a function to handle the incoming tokens, it can result in the loss of tokens.

Proof of Concept

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

_rewardTokens[i].transfer(user, rewardAmount);

Line https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L457

IERC20(rewardsToken).transferFrom(msg.sender, address(adminProxy), amount);

Line https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L526

rewardTokens[i].transferFrom(msg.sender, address(this), amounts[i]);

Tools Used

Manual analysis

Recommended Mitigation Steps

The recommended solution is to use the safeTransfer and safeTransferFrom functions from OpenZeppelin's contracts library, which provide the necessary safety checks to ensure the transfer of tokens is successful and secure.

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor confirmed

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Out of scope

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