Popcorn contest - Kumpa's results

Lines of code

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

Vulnerability details

Impact

claimRewards in multirewardstaking.sol does not follow check-effect-interaction patterns, because in claimRewards, accruedRewards[user][_rewardTokens[i]] is set to zero after rewardToken is transferred to the user.

Normally erc20 will not cause reentrancy but if the reward token is customized token or erc777 with hook calling for onReceive from the caller, then it will be possible to cause the looping draining of reward tokens in the contract.

Recommended Mitigation Steps

A little bit of adjustment as in the picture below

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L134-L158

Vulnerability details

Summary

If the user submits a very small amount of tokens, the user will not receive any shares back and the asset will be locked up in the external protocol.

Proof of Concept

For example, token deposited: asset() = 3, while totalSupply() = 1000, and totalAsset() is mutated to 5000 due to yield return from adapter.

1.User deposit 3 amounts of token a while totalSupply() = 1000, and totalAsset() = 5000

2.The deposited token will converted to shares through the formula below

3.The resulting shares would be (3*1000)/5000 = 0.6 >> rounding down to 0.

4.The token asset will then be transferred from the user to vault to adapter and to protocol.

5.User receive 0 share due to rounding to zero in convertToShares()

Mitigation

Add require(shares != 0, “zero shares”); after the asset is converted to shares.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L143-L145

Vulnerability details

Summary

Since the vault’s owner can arbitrarily create a vault that supports any token, there may be some case where the owner sets the vault asset to be a non-18-decimal token such as USDC with 6 decimals.

This may cause problems to arise because, in the vault, fees for deposit, withdraw, mint, and redeem in vault only account for tokens with 18 decimals in its calculation.

If the vault is for lower decimal tokens, the owner could get a lower fee than expected, and if the vault supports tokens with higher decimal, the fee could be unexpectedly high.

Mitigation

Should change 1e18 to asset.decimal() which will adjust the fee to be in line with the nature of the token deposited.

Lines of code

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

Vulnerability details

Impact

In MultiRewardStaking.sol, the owner could add any amount of new rewardTokens so long as they don’t exceed maximum size of dynamic arrays.

The size of rewardTokens.length however may not match maximum value of uint 8 i in for-loop within the modifier accrueRewards, causing the execution to revert

This is a serious issue because modifier accrueRewards has been used along with all token-entry and token-exit functions, such as _deposit(), _withdraw(). If the modifier is broken, then all these functions will also be broken. If there is token deposited in the contract, it will be locked forever.

Mitigation

There should be a limitation on the maximum numbers of rewardToken and the protocol should also take gas limit per block into account as well since this modifier really consumes an expensive amount of gas, considering that it has been used over and over in all important functions.