Use safeTransferFrom and safeApprove foe the reward tokens in Vault Controller instead of transferFrom and approve functions

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L171 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L456-L457 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L526

Vulnerability details

Impact

In 3 functions return values of ERC20 contracts (either approve or transferFrom) are not checked : fundStakingRewards ; addStakingRewardsTokens and _handleInitialDeposit. This is especially dangerous in the addStakingRewardsTokens function, because in this case, the newly added reward token would be considered as one of the reward tokens, even if the return of its transferFrom function would be False. It could then be a way to steal this same token from the AdminProxy via the adminProxy.execute function which is called right after. However, impact is rather limited because caller of addStakingRewardsTokens must be the creator of the Vault or the owner and furthermore there is a check on the token which must be pre-approved via the call to _verifyToken.

Proof of Concept

If the token returns False, the ERC20-related transactions would have failed but in pratice, a call to addStakingRewardsTokens would succeed, which is not expected.

Recommended Mitigation Steps

Use the safeTransferFrom and safeApprove functions from the SafeERC20 library accordingly.

[c4-sponsor]

RedVeil marked the issue as sponsor disputed

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as unsatisfactory: Out of scope