Popcorn contest - 0xRobocop's results

Lines of code

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

Vulnerability details

Impact

The changeRewardSpeed function from the MultiRewardStaking.sol contract lacks documentation on how exactly it should work. By its name and some comments above it, I infer that the function must change the rate of tokens rewards per unit of time.

For simplicity the context is the following: the function changeRewardSpeed is called with the parameters: tokenA, 2 tokens/second. The state of the staking of tokenA at that point is:

(1) It still has 50 seconds more to go. (2) Its current speed is 1 token / s.

Assume that the balance of tokenAs of the contract at that point is enough to fullfill the obligation of paying all the users its rewards until the end of the staking (this was enforced during the creation of the staking). But that amount was calculated based on a speed, the 1 token / second one. Because we want to increase it to 2 tokens / second, the contract must take from the caller the amount of tokens that is equal to (speed2 - speed1) * time_to_go. In other words it must multiply the difference of speeds by how much time is left, and take that tokens from the caller.

If it were to decrease the speed, then the tokenA's balance of the contract is enough.

So basically the function must change the state in three ways:

(1) Update the accrued rewards to take into account the time from the lastupdate (2) Update the variable rewardsPerSecond (3) If the change was an increment then take from the caller the tokens needed to complete the amount. If it is a decrease, then don't do this last step.

When reading the current implementation of the changeRewardSpeed function it has a different behavior.

It not only changes the rewardPerSecond variable but also the endRewardTimestamp variable using the _calcRewardsEnd function. Problems:

(1) The function name states that the only change must be the speed of rewards not the duration of the staking. (2) The end state after executing the current implementation of changeRewardSpeed function is invalid and can cause a temporally DoS on the staking of token A.

Explaining (2):

The problem is using the _calcRewardsEnd function. This function is used properly at the creation of the staking to compute the end timestamp, it is also used correctly on the fundReward function that has the intention to add more rewards with the same speed, so, the end timestamp must be increased.

In the case of the changeRewardSpeedfunction, the _calcRewardsEnd function is called with the parameters: current endTimestamp, the new speed and the contract's tokenA balance. The semantics of _calcRewardsEnd is the following:

If it is called when the staking is created, simply compute the endTimestamp to be able to distribute amount of tokens given a rewardsPerSecond speed.

If it is called in the middle of a staking (like when using the fundReward function) is first add to amount the amount of tokens that are left to be rewarded and then with the new amount calculate the new endTimestamp.

So the effect of _calcRewardsEnd on changeRewardSpeed is that it will calculate a new endRewardTimestamp like it will take the contract's tokenA balance as the funds to increase rewards and it will calculate the rewards that are left to be given using the new speed.

The Denial of Service will happen because of two things:

(1) The function does not take the surplus of tokens from the caller (2) Even if it enforces the transfer of the surplus of tokens, that surplus is miscalculated because it does not take into account the tokens that are in debt from the last index of each user to the time the changeRewardSpeed function was called.

Tools Used

Manual Review

Recommended Mitigation Steps

Rewrite the function as:

function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner {
    RewardInfo memory rewards = rewardInfos[rewardToken];

    if (rewardsPerSecond == 0) revert ZeroAmount();
    if (rewards.lastUpdatedTimestamp == 0) revert RewardTokenDoesntExist(rewardToken);
    if (rewards.rewardsPerSecond == 0) revert RewardsAreDynamic(rewardToken);

    _accrueRewards(rewardToken, _accrueStatic(rewards));

    // The speed is increasing
    if(rewards.rewardsPerSecond < rewardsPerSecond) {
        extraAmount = (rewardsPerSecond - rewards.rewardsPerSecond) * (rewards.rewardsEndTimestamp - block.timestamp);
        rewardToken.safeTransferFrom(msg.sender, address(this), extraAmount);
        rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
    }

    // The speed is decreasing
    else {
        rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
        // It might reimburse the caller some tokens.
    }
  }

Lines of code

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

Vulnerability details

Impact

ERC777 has transfer hooks before and after the actual transfer of funds. These hooks can be used to take control of the flow of execution, and execute arbitrary logic, like reentering a contract.

The claimRewards function on the MultiRewardStaking.sol contract is implemented as follows:

function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
      uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

      if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);

      EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];

      if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

      accruedRewards[user][_rewardTokens[i]] = 0;
    }
  }

As can be seen, the statement accruedRewards[user][_rewardTokens[i]] = 0; that clears the rewardBalance of the user is left at the end. So when the function is reentered the next check will pass:

uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);

Allowing the attacker to drain all the rewards balance (the one belonging to the ERC777 token) of the contract.

Tools Used

Manual review

Recommended Mitigation Steps

Use a mutex on the function, so it cannot be reentered or move the statement accruedRewards[user][_rewardTokens[i]] = 0; before the transfer, following The Checks-Effects-Interactions pattern.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L667 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L667 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L608 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L618 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L631 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L644

Vulnerability details

Impact

The _verifyCreatorOrOwner(address vault) function at the VaultController.sol contract is used to validate that the msg.sender is the creator of the vault or is the owner of the contract.

It looks like this:

function _verifyCreatorOrOwner(address vault) internal returns (VaultMetadata memory metadata) {
    metadata = vaultRegistry.getVault(vault);
    if (msg.sender != metadata.creator || msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);
}

The logic clause is wrong to achieve that purpose. The only way the function wont revert is if the msg.sender is both the creator of the vault and the owner of the VaultController contract.

The affected functions are: addStakingRewardsTokens, pauseAdapters, pauseVaults, unpauseAdapters and unpauseVaults.

There is no loss of funds for the users, but the functionality of the protocol as was designed would be affected

Tools Used

Manual Review

Recommended Mitigation Steps

Change the logic-or for a logic-and:

function _verifyCreatorOrOwner(address vault) internal returns (VaultMetadata memory metadata) {
    metadata = vaultRegistry.getVault(vault);
    if (msg.sender != metadata.creator && msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);
}

Lines of code

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

Vulnerability details

Impact

The changeRewardSpeed function from the MultiRewardStaking.sol contract lacks documentation on how exactly it should work. By its name and some comments above it, I infer that the function must change the rate of tokens rewards per unit of time.

For simplicity the context is the following: the function changeRewardSpeed is called with the parameters: tokenA, 2 tokens/second. The state of the staking of tokenA at that point is:

(1) It still has 50 seconds more to go. (2) Its current speed is 1 token / s.

Assume that the balance of tokenAs of the contract at that point is enough to fullfill the obligation of paying all the users its rewards until the end of the staking (this was enforced during the creation of the staking). But that amount was calculated based on a speed, the 1 token / second one. Because we want to increase it to 2 tokens / second, the contract must take from the caller the amount of tokens that is equal to (speed2 - speed1) * time_to_go. In other words it must multiply the difference of speeds by how much time is left, and take that tokens from the caller.

If it were to decrease the speed, then the tokenA's balance of the contract is enough.

So basically the function must change the state in three ways:

(1) Update the accrued rewards to take into account the time from the lastupdate (2) Update the variable rewardsPerSecond (3) If the change was an increment then take from the caller the tokens needed to complete the amount. If it is a decrease, then don't do this last step.

When reading the current implementation of the changeRewardSpeed function it has a different behavior.

It not only changes the rewardPerSecond variable but also the endRewardTimestamp variable using the _calcRewardsEnd function. Problems:

(1) The function name states that the only change must be the speed of rewards not the duration of the staking. (2) The end state after executing the current implementation of changeRewardSpeed function is invalid and can cause a temporally DoS on the staking of token A.

Explaining (2):

The problem is using the _calcRewardsEnd function. This function is used properly at the creation of the staking to compute the end timestamp, it is also used correctly on the fundReward function that has the intention to add more rewards with the same speed, so, the end timestamp must be increased.

In the case of the changeRewardSpeedfunction, the _calcRewardsEnd function is called with the parameters: current endTimestamp, the new speed and the contract's tokenA balance. The semantics of _calcRewardsEnd is the following:

If it is called when the staking is created, simply compute the endTimestamp to be able to distribute amount of tokens given a rewardsPerSecond speed.

If it is called in the middle of a staking (like when using the fundReward function) is first add to amount the amount of tokens that are left to be rewarded and then with the new amount calculate the new endTimestamp.

So the effect of _calcRewardsEnd on changeRewardSpeed is that it will calculate a new endRewardTimestamp like it will take the contract's tokenA balance as the funds to increase rewards and it will calculate the rewards that are left to be given using the new speed.

The Denial of Service will happen because of two things:

(1) The function does not take the surplus of tokens from the caller (2) Even if it enforces the transfer of the surplus of tokens, that surplus is miscalculated because it does not take into account the tokens that are in debt from the last index of each user to the time the changeRewardSpeed function was called.

Tools Used

Manual Review

Recommended Mitigation Steps

Rewrite the function as:

function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner {
    RewardInfo memory rewards = rewardInfos[rewardToken];

    if (rewardsPerSecond == 0) revert ZeroAmount();
    if (rewards.lastUpdatedTimestamp == 0) revert RewardTokenDoesntExist(rewardToken);
    if (rewards.rewardsPerSecond == 0) revert RewardsAreDynamic(rewardToken);

    _accrueRewards(rewardToken, _accrueStatic(rewards));

    // The speed is increasing
    if(rewards.rewardsPerSecond < rewardsPerSecond) {
        extraAmount = (rewardsPerSecond - rewards.rewardsPerSecond) * (rewards.rewardsEndTimestamp - block.timestamp);
        rewardToken.safeTransferFrom(msg.sender, address(this), extraAmount);
        rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
    }

    // The speed is decreasing
    else {
        rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
        // It might reimburse the caller some tokens.
    }
  }

Lines of code

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

Vulnerability details

Impact

The feeRecipient of the MultiRewardEscrow.sol contract is used as the address to receive any fee from the vesting of staking rewards.

The vulnerability is that this address cannot be updated even by the owner. If an incorrect address is set, all the fees generated will be lost. The other case is that the address was correctly set, but one of the following two things can happen:

If the account is an EOA it can be the case that it gets compromised and hence, all the fees earned and also the ones that will be generated in the future will also be compromised.

If the account is a contract, then it can have vulnerabilities in it that don't make it reliable.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow the owner to change the feeRecipient variable

[L-01] Incorrect trigger of events can cause confusion on off-chain monitors.

The _transfer function of the MultiRewardStaking.sol contract emits three events of Transfer, one for the actual transfer and two more because it uses the internal functions _burn and _mint which also emits events of Transfer.

Actual transfer emit Transfer(from, to, amount); Mint emit Transfer(address(0), to, amount); Burn emit Transfer(from, address(0), amount);

Off-chain monitors can get confused because of this. For example: Subtracting two times amount of tokens from from and adding two times amount to to.

[L-02] Initializing multiple MultiRewardStaking contracts with the same _stakingToken can lead to confusion.

When a MultiRewardStaking contract is initialized its body of code contains:

_name = string(abi.encodePacked("Staked ", IERC20Metadata(address(_stakingToken)).name()));
_symbol = string(abi.encodePacked("pst-", IERC20Metadata(address(_stakingToken)).symbol()));

It writes to the _name and _symbol state variables based on the _stakingToken variable. If more than one staking contract uses the same _stakingToken they will have the same _name and _symbol, which can lead to confusion.

There is another instance of this on the initialize function of the Vault.sol contract.

[L-03] Is safer to cast block.timestamp to uint64 instead of uint32

Instances:

LoC 114 MultiRewardEscrow LoC 163 MultiRewardEscrow LoC 175 MultiRewardEscrow

[L-04] Disable initializers on the implementations contracts.

Context: Here

Contracts that should disable initializers:

Vault.sol, VaultController.sol, YearnAdapter.sol and BeefyAdapter

[L-05] The Vault.sol contract does not inherit the IERC4626 interface

It is important to inherit the interface, so the compliance with the interface can be confirmed by the compiler, and also to add it to one of the supported interfaces (as EIP165).

[N-01] Remove events or errors not used

Some instances found:

event TemplateUpdated(bytes32, bytes32) at the TemplateRegistry.sol contract. error NotEndorsed(bytes32) at the CloneFactory.sol contract event NoFee() at the MultiRewardEscrow contract

[N-02] Some initialize functions of inherited upgreadable contract are not been called.

Vault.sol initialize contract does not call the initialize functions of the Pausable and Reentrancy contracts

[N-03] NatSpec comments are incorrect.

DeploymentController.sol: LoC 60 it copied the comment for addTemplateCategory. Here is not a Category that is being added, but a Template.

TemplateRegistry.sol:
	LoC 23 it should say 'DeploymentController' instead of AdminProxy.
	
CloneRegistry.sol:
	LoC 21 it should say 'DeploymentController' instead of AdminProxy.
	
CloneFactory.sol
	LoC 22 it should say 'DeploymentController' instead of AdminProxy.