Popcorn contest - 0xMirce's results

Lines of code

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

Vulnerability details

Impact

supplierDelta calculation is used to calculate the amount of reward tokens that users could claim. If the number of decimals for reward tokens is different than the decimals for stakingToken (that are and decimals for the MultiRewardStaking contract), the amount of reward tokens that the user could claim will be more or less than it should be. That difference could be especially seen in the case where decimals for the MultiRewardStaking contract are 18 decimals, and for reward is 6 decimals (USDC or USDT, for example).

Proof of Concept

Below is an example where the MultiRewardStaking contract has 18 decimals, and the reward token created and used has 6 decimals.

    MockERC20 rewardTokenTest = new MockERC20("RewardsTokenTest", "RTKNT", 6);
    IERC20 iRewardTokenTest = IERC20(address(rewardTokenTest));
    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardTokenTest;

    _addRewardToken(rewardTokenTest);
    stakingToken.mint(alice, 5 ether);

    vm.startPrank(alice);
    stakingToken.approve(address(staking), 5 ether);
    staking.deposit(1 ether);

    // 10% of rewards paid out
    vm.warp(block.timestamp + 10);

    staking.deposit(1); // Only to trigger modifier `accrueRewards`
    
    assertEq(staking.accruedRewards(alice, iRewardTokenTest), 1000000000000 ether);

As it is seen from this example, the reward amount that the user will get is 1000000000000 ether which is a lot higher than it should be (an actual amount that for this setup should be 1 ether). Also, here is an example where rewardToken is also 18 decimals, the same as the MultiRewardStaking contract.

    MockERC20 rewardTokenTest = new MockERC20("RewardsTokenTest", "RTKNT", 18);
    IERC20 iRewardTokenTest = IERC20(address(rewardTokenTest));
    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardTokenTest;

    _addRewardToken(rewardTokenTest);
    stakingToken.mint(alice, 5 ether);

    vm.startPrank(alice);
    stakingToken.approve(address(staking), 5 ether);
    staking.deposit(1 ether);

    // 10% of rewards paid out
    vm.warp(block.timestamp + 10);

    staking.deposit(1); // Only to trigger modifier `accrueRewards`
    
    assertEq(staking.accruedRewards(alice, iRewardTokenTest), 1 ether);

Tools Used

VSCode, Foundry, Solidity Visual Developer

Recommended Mitigation Steps

The critical problem for this calculation is that the formula uint256 supplierDelta = balanceOf(_user).mulDiv(deltaIndex, uint256(rewards.ONE), Math.Rounding.Down); uses balanceOf from the current contract which result has, of course, same decimals as MultiRewardStaking contract. On another side, for division, it uses uint256(rewards.ONE) which has a number of decimals for the reward token. A suggestion is to calculate supplierDelta into a number of decimals for the MultiRewardStaking contract, and after that, the supplierDelta variable only converts to the number of decimals of rewardToken.

First change

    rewardInfos[rewardToken] = RewardInfo({
        ONE: ONE,
        rewardsPerSecond: rewardsPerSecond,
        rewardsEndTimestamp: rewardsEndTimestamp,
        index: (10**decimals()).safeCastTo64(),
        lastUpdatedTimestamp: block.timestamp.safeCastTo32()
    });

Second change

    function _accrueUser(address _user, IERC20 _rewardToken) internal {
        RewardInfo memory rewards = rewardInfos[_rewardToken];

        uint256 oldIndex = userIndex[_user][_rewardToken];

        // If user hasn't yet accrued rewards, grant them interest from the strategy beginning if they have a balance
        // Zero balances will have no effect other than syncing to global index
        uint256 defaultDecimals = (10**decimals());
        if (oldIndex == 0) {
        oldIndex = defaultDecimals;
        }

        uint256 deltaIndex = rewards.index - oldIndex;

        // Accumulate rewards by multiplying user tokens by rewardsPerToken index and adding on unclaimed
        uint256 supplierDelta = balanceOf(_user).mulDiv(deltaIndex, defaultDecimals, Math.Rounding.Down);
        supplierDelta = supplierDelta.mulDiv(rewards.ONE, defaultDecimals);

        userIndex[_user][_rewardToken] = rewards.index;

        accruedRewards[_user][_rewardToken] += supplierDelta;
    }

Lines of code

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

Vulnerability details

Impact

The formula for calculation rewardEndTimestamp when called from the changeRewardSpeed function is not good. Both for increase or decrease reward speed, a formula for calculation rewardEndTimestamp add additional time, so the key impact is that after some time, users will stop to get staking rewards, or even worse, if all users wait for rewardEndTimestamp, it will not have enough reward token that all users be paid (so someone will get more amount that should be, and other will not get anything).

Proof of Concept

Test and example when speed is decreased (from 0.1 to 0.05 reward per second)

    vm.warp(block.timestamp + 10);
   
    fillInitialData();

    assertEq(block.timestamp, 11);

    // 10% of rewards paid out
    vm.warp(block.timestamp + 10);
    assertEq(block.timestamp, 21);

    (, uint160 rewardsPerSecond, uint32 rewardsEndTimestamp, , ) = staking.rewardInfos(IERC20(address(rewardToken1)));
    assertEq(rewardsPerSecond, 100000000000000000); // 0.1 per second
    assertEq(rewardsEndTimestamp, 111);

    staking.changeRewardSpeed(iRewardToken1, 0.05 ether);

    (, rewardsPerSecond, rewardsEndTimestamp, , ) = staking.rewardInfos(IERC20(address(rewardToken1)));
    assertEq(rewardsPerSecond, 50000000000000000); // 0.1 per second
    assertEq(rewardsEndTimestamp, 311); // WRONG VALUE, SHOULD BE 201!

Initially, the start time was in 11 second, the end time was at 111 second, and it was 10 tokens for reward. Then, 10 seconds later (at 21 second) was called changeRewardSpeed, so by that moment, 1 reward token was given as a reward, and 9 tokens were left in the contract. If speed decreases from 0.1 to 0.05 tokens per second, that means that for 9 tokens, it will have 180 seconds to share all reward tokens. Also, because a function changeRewardSpeed is called at 21 second, that means that rewardsEndTimestamp should be 201 (and not 311).

A similar situation is also when speed is increased (in this example, from 0.1 to 0.2 reward per second)

 vm.warp(block.timestamp + 10);
    _addRewardToken(rewardToken1);
 
    fillInitialData();

    assertEq(block.timestamp, 11);

    // 10% of rewards paid out
    vm.warp(block.timestamp + 10);
    assertEq(block.timestamp, 21);

    (, uint160 rewardsPerSecond, uint32 rewardsEndTimestamp, , ) = staking.rewardInfos(IERC20(address(rewardToken1)));
    assertEq(rewardsPerSecond, 100000000000000000); // 0.1 per second
    assertEq(rewardsEndTimestamp, 111);

    staking.changeRewardSpeed(iRewardToken1, 0.2 ether);

    (, rewardsPerSecond, rewardsEndTimestamp, , ) = staking.rewardInfos(IERC20(address(rewardToken1)));
    assertEq(rewardsPerSecond, 200000000000000000); // 0.2 per second
    assertEq(rewardsEndTimestamp, 161); // WRONG VALUE, SHOULD BE 66!

Same as the previous example for decrease, initially, the start time was in 11 second, the end time was at 111 second, and it was 10 tokens for reward. Then, 10 seconds later (at 21 second) was called changeRewardSpeed, so by that moment, 1 reward token was given as a reward, and 9 tokens were left in the contract. If speed increases from 0.1 to 0.2 tokens per second, that means that for 9 tokens, it will have 45 seconds to share all reward tokens. Also, because a function changeRewardSpeed is called at 21 second, that means that rewardsEndTimestamp should be 66 (and not 161).

Tools Used

VSCode, Foundry, Solidity Visual Developer

Recommended Mitigation Steps

Change the formula for calculation rewardsEndTimestamp and a new formula should be

uint256 oldRewardsPerSecond = rewards.rewardsPerSecond;
uint256 newRewardsPerSecond = rewardsPerSecond;
uint256 remainingRewards = uint256(oldRewardsPerSecond) * (rewards.rewardsEndTimestamp - block.timestamp);
uint256 rewardsEndTimestamp =  block.timestamp + remainingRewards / newRewardsPerSecond;

Also, it should be very careful if wants to add this calculation in the _calcRewardsEnd function, because that function is also used in the addRewardToken and fundReward functions.

Judge has assessed an item in Issue #523 as 2 risk. The relevant finding follows:

Title Add function for feeRecipient change in MultiRewardEscrow.sol contract

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

Vulnerability details Impact If account feeRecipient would be compromised, or the protocol owner wants from some other reason to change this address, it must be setter function setFeeRecipient.

Tools Used Manual reviewing

Recommended Mitigation Steps function setFeeRecipient(address _feeRecipient) external onlyOwner { if (_feeRecipient == address(0)) revert ZeroAddress(); feeRecipient = _feeRecipient; }

Lines of code

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

Vulnerability details

Impact

The function addRewardToken() allows the owner to add a new reward token to the list rewardTokens. However, this is an unbounded list that when appended to cannot be shortened.

The impact is it is possible to reach a state where the list is so long it cannot be iterated through due to the gas cost being larger than the block gas limit. This would cause a state where all transactions which iterate over this list will revert.

Since the modifier accrueRewards() iterates over this list it is possible that there will reach a state where the we are unable to call any functions with this modifier. The list includes

• deposit()
• mint()
• withdraw()
• redeem()
• transfer()
• tranferFrom()
• claimRewards()

As a result it would therefore be impossible to withdraw any rewards from this contract.

Proof of Concept

Flow is next:

1. Add 255 (or very possibly a lot fewer) reward tokens in the staking contract
2. Call one of the functions from the list above

    for(uint8 counter; counter < 255; counter++) {
      MockERC20 rewardToken = new MockERC20("RewardsToken", "RTKN", 18);
      IERC20 iRewardToken = IERC20(address(rewardToken));
      rewardToken.mint(address(this), 10 ether);
      rewardToken.approve(address(staking), 10 ether);
      staking.addRewardToken(iRewardToken, 0.1 ether, 10 ether, true, 10000000, 100, 20);
    }
    stakingToken.mint(alice, 1 ether);
    vm.startPrank(alice);
    stakingToken.approve(address(staking), 1 ether);
    staking.deposit(1 ether);
    vm.stopPrank();

Tools Used

VSCode, Foundry, Solidity Visual Developer

Recommended Mitigation Steps

Consider having some method for removing old reward tokens which are no longer in use. Alternatively set a hard limit on the number of reward tokens that can be added.

A different option is to allow rewards to be iterated and distributed on a per token bases rather than all tokens at once.

Title

getSubmitter function returns whole vault struct and not only vault.creator

Links to affected code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultRegistry.sol#L75

Vulnerability details

Tools Used

Manual reviewing

Recommended Mitigation Steps

    function getSubmitter(address vault) external view returns (address) {
        return metadata[vault].creator;
    }

Title

Add function for feeRecipient change in MultiRewardEscrow.sol contract

Links to affected code

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

Vulnerability details

Impact

If account feeRecipient would be compromised, or the protocol owner wants from some other reason to change this address, it must be setter function setFeeRecipient.

Tools Used

Manual reviewing

Recommended Mitigation Steps

    function setFeeRecipient(address _feeRecipient) external onlyOwner {
        if (_feeRecipient == address(0)) revert ZeroAddress();
        feeRecipient = _feeRecipient;
    }

Title

Missing check is template.implementation different than address(0) in function addTemplate

Links to affected code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/TemplateRegistry.sol#L67

Vulnerability details

Impact

When the contract owner tries to clone the template where the implementation address is 0, it will get an error. The additional problem is that there is no template update function, so there is not possible to change the implementation address.

Tools Used

Manual reviewing

Recommended Mitigation Steps

    function addTemplate(
        bytes32 templateCategory,
        bytes32 templateId,
        Template memory template
    ) external onlyOwner {
        if (!templateCategoryExists[templateCategory]) revert KeyNotFound(templateCategory);
        if (templateExists[templateId]) revert TemplateExists(templateId);
        if (template.implementation == address(0)) revert ZeroAddress();

        template.endorsed = false;
        templates[templateCategory][templateId] = template;

        templateIds[templateCategory].push(templateId);
        templateExists[templateId] = true;

        emit TemplateAdded(templateCategory, templateId, template.implementation);
    }

Title

Unnecessary _verifyEqualArrayLength

Links to affected code

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

Vulnerability details

Impact

In line MultiRewardEscrow.sol:208 there is a same check, so, VaultController.sol#L544 is not needed and could be removed

Tools Used

Manual reviewing

Recommended Mitigation Steps

Remove line VaultController.sol#L544

Title

Unnecessary lastUpdatedTimestamp update in fundReward function

Links to affected code

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

Vulnerability details

Impact

In line MultiRewardStaking.sol#L339 is called function _accrueRewards where there is a same update MultiRewardStaking.sol#L409

Tools Used

Manual reviewing

Recommended Mitigation Steps

Remove line MultiRewardStaking.sol#L346

Title

Function state mutability can be restricted to view

Links to affected code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L351 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L242 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L667

Title

Unused local variable

Links to affected code

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

Title

Unused function parameter

Links to affected code

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

Title

Return value of low-level calls not used

Links to affected code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L444

Title

Declaration shadows an existing declaration

Links to affected code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L388 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L124 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L446 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L60 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L176 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L196 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L213 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L633 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L62 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L214 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L256 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L665

Vulnerability details

Recommended Mitigation Steps

Rename variables from code lines from section Links to affected code