Popcorn contest - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The first depositor can inflate the price of shares and steal funds from future depositors.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

First, although the VaultController has the _handleIntialDeposit() function below, it does not prevent first depositor attack because 1) initialDeposit could be zero, that is, the creator of the vault might choose deposit zero initially; 2) the owner might be malicious and be the one who launches the first depositor attack, thus, _handleIntialDeposit() can be used just for that purpose; 3) initialDeposit is small, then attackers can still launch first depositor attack.

function _handleInitialDeposit(
    uint256 initialDeposit,
    IERC20 asset,
    IERC4626 target
  ) internal {
    if (initialDeposit > 0) {
      asset.safeTransferFrom(msg.sender, address(this), initialDeposit);
      asset.approve(address(target), initialDeposit);
      target.deposit(initialDeposit, msg.sender);
    }
  }

In the following, we assume initialDeposit = 0, the other two cases are similar in analysis.

1. Suppose attacker Bob is the first depositor (by front-running if necessary) and deposits 1 wei of asset and gets 1 share, using the following deposit() function:

function deposit(uint256 assets, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 shares)
    {
        if (receiver == address(0)) revert InvalidReceiver();

        uint256 feeShares = convertToShares(
            assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)
        );

        shares = convertToShares(assets) - feeShares;

        if (feeShares > 0) _mint(feeRecipient, feeShares);

        _mint(receiver, shares);

        asset.safeTransferFrom(msg.sender, address(this), assets);

        adapter.deposit(assets, address(this));

        emit Deposit(msg.sender, receiver, assets, shares);
    }

function convertToShares(uint256 assets) public view returns (uint256) {
        uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

        return
            supply == 0
                ? assets
                : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
    }

2. Bob than sends 1000x1e18 asset tokens to the adapter, then send the shares returned from the adapter to the vault. As a result, Bob inflates the price of per share of the vault to 1000x1e18+1/share. Note: we need to send the assets tokens to the adapter instead of to the vault because the way that vault counts totalAsset() below - it counts the total asset at the adapter, not at the vault itself!

function totalAssets() public view returns (uint256) {
        return adapter.convertToAssets(adapter.balanceOf(address(this)));
    }

3. Alice deposits 2000x1e18 asset tokens to the vault, and gets 2000*1e18/(1000*1e18+1) = 1 share as well.

4. The price of per share is changed to (3000x1e18+1)/2 = 1500x1e18.

5. Bob calls redeem() to redeem his one share and gets 1500x1e18 asset tokens, gaining 500x1e18 tokens;

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public nonReentrant returns (uint256 assets) {
        if (receiver == address(0)) revert InvalidReceiver();

        if (msg.sender != owner)
            _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);

        uint256 feeShares = shares.mulDiv(
            uint256(fees.withdrawal),
            1e18,
            Math.Rounding.Down
        );

        assets = convertToAssets(shares - feeShares);

        _burn(owner, shares);

        if (feeShares > 0) _mint(feeRecipient, feeShares);

        adapter.withdraw(assets, receiver, address(this));

        emit Withdraw(msg.sender, receiver, owner, assets, shares);
    }

6. If Alice also chose to redeem at this point, she will only receive 1500 x 1e18 asset tokens. She lost 500 x 1e18 asset tokens. Bob stole them from her!

Tools Used

Remix

Recommended Mitigation Steps

A few strategies to prevent first depositor attack:

1. Enforce a minimum 1000 wei asset deposit, this is not a high requirement for most tokens but can be adjusted based on the decimals of the asset token to make it large enough but still affordable. This will make price inflation pretty costing. Also a reasonable filter for good-faith investors.

2. Lock 1000 shares and 1000 wei of assets in the contract forever from the first deposit. 1000 wei is a nominal fee for most asset tokens. This can prevent future first depositor attacks.

3. Make a mandatory first deposit as part of the deployment of the vault and deposit at least 1000 wei or more.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Ragequit enforcement is important in two aspects at least: 1) To ensure customers can have the time to adapt and prepare for the upcoming changes; 2) To have the time to mitigate if some changes are made by malicious attackers or compromised owner. However, setQuitPeriod() allows the owner to change the quitPeriod without rageperiod enforcement itself. That is, the new quitPeriod will take effect immediately.

Impacts:

1. Customers might have not time to respond, adapt and prepare for the new quitPeriod.
2. A compromised/malicious owner might manipulate or even bypass existing ragequit constraint to introduce malicious components into the protocol.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The setQuitPeriod() function is implemented without a ragequit control itself; see below

function setQuitPeriod(uint256 _quitPeriod) external onlyOwner {
        if (_quitPeriod < 1 days || _quitPeriod > 7 days)
            revert InvalidQuitPeriod();

        quitPeriod = _quitPeriod;

        emit QuitPeriodSet(quitPeriod);
    }

As a result, the new quitPeriod will take effect immediately. These will create two bad impacts:

1. When a new shortened quitPeriod is proposed, it will be in effect immediately. Many changes might become immediately possible. For example, a proposal is just passed to change the adapter in 7 days, then suddenly, the quitPeriod is changed to 1 day by the setQuitPeriod() function , as a result, the adapter is changed on the second day. Customers only expect the adapter will be introduced in 7 days, not in 1 day. This will be a shock to the customers. They won't have enough time to adapt, prepare and respond to the sudden unexpected introduction of a new adapter. This is against the purpose of ragequit control.

2. Suppose the current quitPeriod is 7 days. A malicious/compromised owner is eager to use a new malicious adapter, so he calls setQuitPeriod() to set the new quitperiod to 1 day, insert the malicious adapter is inserted, then call setQuitPeriod() to set quitPeriod back to 7 days to avoid being caught. The malicious/compromised owner effectively bypassed the original quitrage control to impute his malicious adapter into the system.

Tools

Remix

Recommended Mitigation Steps

Enforce the ragequit control on setQuitPeriod() as well. The period for setQuitPeriod() can be set as a constant to prevent manipulation.

function proposeQuitPeriod(uint256 _quitPeriod) external onlyOwner {
        if (_quitPeriod < 1 days || _quitPeriod > 7 days)
            revert InvalidQuitPeriod();

        if(proposedQuitPeriod == quitPeriod) revert NotANewQuitPeriod();
        proposedQuitPeriod = _quitPeriod;
        proposedQuitPeriodTime = block.timestamp;

        emit QuitPeriodProposed(quitPeriod, block.timestamp);
    }

function setQuitPeriod() external onlyOwner {

        if(block.timestamp < proposedQuitPeriodTime + 7 days) revert TooEarlyToChangeQuitPeriod();
        quitPeriod = proposedQuitPeriod;

        emit QuitPeriodChanged(proposedQuitPeriod, block.timestamp);

        emit QuitPeriodProposed(quitPeriod, block.timestamp);
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

DeploymentController must be the owner for the Clonefactory, CloneRegistry and TemplateRegsitry for the protocol to work. Unfortunately, after new DeploymentController is introduced by _setDeploymentController(), it is impossible to make it become the new owner for existing Clonefactory, CloneRegistry and TemplateRegsitry. _setDeploymentController() itself does not change such owner relationship. As a result, the new DeploymentController will not function in the protocol.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

First, we need to understand that DeploymentController must be the owner for the Clonefactory, CloneRegistry and TemplateRegsitry contracts. We can see this because DeploymentController will call the functions in all these three contracts which has theonlyOwner modifier; we can also see this from this diagram given in the repo.

function addTemplate(
    bytes32 templateCategory,
    bytes32 templateId,
    Template calldata template
  ) external {
    templateRegistry.addTemplate(templateCategory, templateId, template);
  }

function deploy(
    bytes32 templateCategory,
    bytes32 templateId,
    bytes calldata data
  ) external onlyOwner returns (address clone) {
    Template memory template = templateRegistry.getTemplate(templateCategory, templateId);

    if (!template.endorsed) revert NotEndorsed(templateId);

    clone = cloneFactory.deploy(template, data);

    cloneRegistry.addClone(templateCategory, templateId, clone);
  }

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

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

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

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

 function deploy(Template calldata template, bytes calldata data) external onlyOwner returns (address clone) {
    clone = Clones.clone(template.implementation);

    bool success = true;
    if (template.requiresInitData) (success, ) = clone.call(data);

    if (!success) revert DeploymentInitFailed();

    emit Deployment(clone);
  }

 function addClone(
    bytes32 templateCategory,
    bytes32 templateId,
    address clone
  ) external onlyOwner {
    cloneExists[clone] = true;
    clones[templateCategory][templateId].push(clone);
    allClones.push(clone);

    emit CloneAdded(clone);
  }

Below we show that, after a new DeploymentController is introduced, it is impossible to make it become the new owner for existing Clonefactory, CloneRegistry and TemplateRegsitry. As a result, the new DeploymentController will not function in the protocol.

1. Suppose the owner of VaultController calls the setDeploymentController() to change DeploymentController from A to B.

  function setDeploymentController(IDeploymentController _deploymentController) external onlyOwner {
    _setDeploymentController(_deploymentController);
  }

  function _setDeploymentController(IDeploymentController _deploymentController) internal {
    if (address(_deploymentController) == address(0) || address(deploymentController) == address(_deploymentController))
      revert InvalidDeploymentController(address(_deploymentController));

    emit DeploymentControllerChanged(address(deploymentController), address(_deploymentController));

    deploymentController = _deploymentController;
    cloneRegistry = _deploymentController.cloneRegistry();
    templateRegistry = _deploymentController.templateRegistry();
  }

2. At this point, the owner of A is the adminProxy, and the owner of adminProxy is VaultController, which can be seen from the following call-chain. Since both adminProxy.execute() and DeploymentController.deploy() have the onlyOwner modifier, so the above owner relationship is needed for the call-chain VaultController._deplloyAdaptor->adminProxy.execute()->_deploymentController.deploy() to succeed.

function __deployAdapter(
    DeploymentArgs memory adapterData,
    bytes memory baseAdapterData,
    IDeploymentController _deploymentController
  ) internal returns (address adapter) {
    (bool success, bytes memory returnData) = adminProxy.execute(
      address(_deploymentController),
      abi.encodeWithSelector(DEPLOY_SIG, ADAPTER, adapterData.id, _encodeAdapterData(adapterData, baseAdapterData))
    );
    if (!success) revert UnderlyingError(returnData);

    adapter = abi.decode(returnData, (address));

    adminProxy.execute(adapter, abi.encodeWithSelector(IAdapter.setPerformanceFee.selector, performanceFee));
  }

contract AdminProxy is Owned {
  constructor(address _owner) Owned(_owner) {}

  /// @notice Execute arbitrary management functions.
  function execute(address target, bytes calldata callData)
    external
    onlyOwner
    returns (bool success, bytes memory returndata)
  {
    return target.call(callData);
  }
}

function deploy(
    bytes32 templateCategory,
    bytes32 templateId,
    bytes calldata data
  ) external onlyOwner returns (address clone) {
    Template memory template = templateRegistry.getTemplate(templateCategory, templateId);

    if (!template.endorsed) revert NotEndorsed(templateId);

    clone = cloneFactory.deploy(template, data);

    cloneRegistry.addClone(templateCategory, templateId, clone);
  }

3. To change the owner of existing Clonefactory, CloneRegistry and TemplateRegsitry from A to B, we have to call A.nominateNewDependencyOwner(B) since A is the current owner of these contracts, but A.nominateNewDependencyOwner() must be called by adminProxy since adminProxy is the owner of A.

function nominateNewDependencyOwner(address _owner) external onlyOwner {
    IOwned(address(cloneFactory)).nominateNewOwner(_owner);
    IOwned(address(cloneRegistry)).nominateNewOwner(_owner);
    IOwned(address(templateRegistry)).nominateNewOwner(_owner);
  }

  /**
   * @notice Accept ownership of dependency contracts.
   * @dev Must be called after construction.
   */
  function acceptDependencyOwnership() external {
    IOwned(address(cloneFactory)).acceptOwnership();
    IOwned(address(cloneRegistry)).acceptOwnership();
    IOwned(address(templateRegistry)).acceptOwnership();
  }

4. The adminProxy() lacks the functions to call A.nominateNewDependencyOwner(B) directly, but there is a relay function execute(), so we have to check its owner, VaultController to see if the owner can call execute() to call the A.nominateNewDependencyOwner(B) to change ownership.

contract AdminProxy is Owned {
  constructor(address _owner) Owned(_owner) {}

  /// @notice Execute arbitrary management functions.
  function execute(address target, bytes calldata callData)
    external
    onlyOwner
    returns (bool success, bytes memory returndata)
  {
    return target.call(callData);
  }
}

5) None of the existing functions in ``VaultController`` allow us to call the ``A.nominateNewDependencyOwner(B)`` through ``adminProxy``.

6) We conclude after new ``DeploymentController`` is introduced, it is impossible to make it become the new owner for existing   ``Clonefactory``, ``CloneRegistry`` and ``TemplateRegsitry``. As a result, the new ``DeploymentController`` will not function in the protocol.  

## Tools Used
Remix

## Recommended Mitigation Steps

1) We need to revise the ``VaultController._setDeploymentController()`` function so that it will call 
``A.nominateNewDependencyOwner(B)`` through the ``adminProxy`` and then call ``B.acceptDependencyOwnership()``. 

2) We can implement a wrapper versions for ``nominateNewDependencyOwner()`` and ``acceptDependencyOwnership()`` that will call their counterparts in ``DeploymentController`` via ``adminProxy``.
 

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. In contrast to withdraw(), depoist(), mint(), which uses the modifier syncFeeCheckpoint to sync the watermark, the redeem() function does not have the syncFeeCheckpoint, as a result,

1. Watermark will be out of sync after redeem() is called.

2. The amount of performance fee and management fee will be improperly calculated.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

In contrast to withdraw(), depoist(), mint(), which uses the modifier syncFeeCheckpoint to sync the watermark, the redeem() function does not have the syncFeeCheckpoint modifier.

function redeem(uint256 shares) external returns (uint256) {
        return redeem(shares, msg.sender, msg.sender);
    }

    /**
     * @notice Burn exactly `shares` vault shares from `owner` and send underlying `asset` tokens to `receiver`.
     * @param shares Quantity of vault shares to exchange for underlying tokens.
     * @param receiver Receiver of underlying tokens.
     * @param owner Owner of burned vault shares.
     * @return assets Quantity of `asset` sent to `receiver`.
     */
    function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public nonReentrant returns (uint256 assets) {
        if (receiver == address(0)) revert InvalidReceiver();

        if (msg.sender != owner)
            _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);

        uint256 feeShares = shares.mulDiv(
            uint256(fees.withdrawal),
            1e18,
            Math.Rounding.Down
        );

        assets = convertToAssets(shares - feeShares);

        _burn(owner, shares);

        if (feeShares > 0) _mint(feeRecipient, feeShares);

        adapter.withdraw(assets, receiver, address(this));

        emit Withdraw(msg.sender, receiver, owner, assets, shares);
    }

The syncFeeCheckpoint function is used to sync the watermark.

modifier syncFeeCheckpoint() {
        _;
        highWaterMark = convertToAssets(1e18);
    }

As a result, the performance fee and management fee will not be properly charged, see below:

 function accruedPerformanceFee() public view returns (uint256) {
        uint256 highWaterMark_ = highWaterMark;
        uint256 shareValue = convertToAssets(1e18);
        uint256 performanceFee = fees.performance;

        return
            performanceFee > 0 && shareValue > highWaterMark
                ? performanceFee.mulDiv(
                    (shareValue - highWaterMark) * totalSupply(),
                    1e36,
                    Math.Rounding.Down
                )
                : 0;
    }

    modifier takeFees() {
        uint256 managementFee = accruedManagementFee();
        uint256 totalFee = managementFee + accruedPerformanceFee();
        uint256 currentAssets = totalAssets();
        uint256 shareValue = convertToAssets(1e18);

        if (shareValue > highWaterMark) highWaterMark = shareValue;

        if (managementFee > 0) feesUpdatedAt = block.timestamp;

        if (totalFee > 0 && currentAssets > 0)
            _mint(feeRecipient, convertToShares(totalFee));

        _;
    }

Tools Used

Remix

Recommended Mitigation Steps

Add syncFeeCheckpoint to the redeem() function as well.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. There is a race condition between changeFees() and takeManagementAndPerformanceFees(). Their order will result in two totally different charges of management fee and performance fee, RETROACTIVELY.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

There is a race condition between changeFees() and takeManagementAndPerformanceFees(). The result depends on their execution order.

 function takeManagementAndPerformanceFees()
        external
        nonReentrant
        takeFees
    {}

    /// @notice Collect management and performance fees and update vault share high water mark.
    modifier takeFees() {
        uint256 managementFee = accruedManagementFee();
        uint256 totalFee = managementFee + accruedPerformanceFee();
        uint256 currentAssets = totalAssets();
        uint256 shareValue = convertToAssets(1e18);

        if (shareValue > highWaterMark) highWaterMark = shareValue;

        if (managementFee > 0) feesUpdatedAt = block.timestamp;

        if (totalFee > 0 && currentAssets > 0)
            _mint(feeRecipient, convertToShares(totalFee));

        _;
    }

function changeFees() external {
        if (block.timestamp < proposedFeeTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);

        emit ChangedFees(fees, proposedFees);
        fees = proposedFees;
    }

1. Consider that the current fees for management is 2% and the current performance fee is 3% and a user A is going to call changeFees() to change both of them to 5%.

2. Another user B front-runs the transaction and execute takeManagementAndPerformanceFees(), as a result, both management and performance fees will be accrued retroactively using 2% and 3%, respectively. After that, changeFees() gets run, and from now on, management fee and performance fee will be both charged at 5%.

3. If takeManagementAndPerformanceFees() was not invoked, then A's invocation of changeFees() would have succeeded and a call of takeManagementAndPerformanceFees() would have used the new 5% for both management fee and performance to accrue for the past (from last update time). As a result, more management and performance fee would have been charged, which is not fair to the users. Since fee parameters should NOT be applied RETROACTIVELY.

Tools Used

Remix

Recommended Mitigation Steps

Add the modifier takeFees to the changeFees() function so that fees can be accrued using old parameters first before the new fee parameters set in.

 function changeFees() takeFees external {
        if (block.timestamp < proposedFeeTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);

        emit ChangedFees(fees, proposedFees);
        fees = proposedFees;
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. syncFeeCheckpoint() does not modify the highWaterMark correctly, sometimes it might even decrease its value, resulting charging more performance fees than it should.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The Vault.syncFeeCheckpoint() function does not modify the highWaterMark correctly, sometimes it might even decrease its value, resulting charging more performance fees than it should. Instead of updating with a higher share values, it might actually decrease the value of highWaterMark. As a result more performance fees might be charged since the highWaterMark was brought down again and again.

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

 modifier syncFeeCheckpoint() {
        _;
        highWaterMark = convertToAssets(1e18);
    }

1. Suppose the current highWaterMark = 2 * e18 and convertToAssets(1e18) = 1.5 * e18.

2. After deposit() is called, since the deposit() function has the synFeeCheckpoint modifier, the highWaterMark will be incorrectly reset to 1.5 * e18.

3. Suppose after some activities, convertToAssets(1e18) = 1.99 * e18.

4. TakeFees() is called, then the performance fee will be charged, since it wrongly decides convertToAssets(1e18) > highWaterMark with the wrong highWaterMark = 1.5 * e18. The correct highWaterMark should be 2 * e18

 modifier takeFees() {
        uint256 managementFee = accruedManagementFee();
        uint256 totalFee = managementFee + accruedPerformanceFee();
        uint256 currentAssets = totalAssets();
        uint256 shareValue = convertToAssets(1e18);

        if (shareValue > highWaterMark) highWaterMark = shareValue;

        if (managementFee > 0) feesUpdatedAt = block.timestamp;

        if (totalFee > 0 && currentAssets > 0)
            _mint(feeRecipient, convertToShares(totalFee));

        _;
    }
function accruedPerformanceFee() public view returns (uint256) {
        uint256 highWaterMark_ = highWaterMark;
        uint256 shareValue = convertToAssets(1e18);
        uint256 performanceFee = fees.performance;

        return
            performanceFee > 0 && shareValue > highWaterMark
                ? performanceFee.mulDiv(
                    (shareValue - highWaterMark) * totalSupply(),
                    1e36,
                    Math.Rounding.Down
                )
                : 0;
    }

5. As a result, the performance fee is charged when it is not supposed to do so. Investors might not be happy with this.

Tools Used

Remix

Recommended Mitigation Steps

Revise the syncFeeCheckpoint() as follows:

 modifier syncFeeCheckpoint() {
        _;
     
         uint256 shareValue = convertToAssets(1e18);

        if (shareValue > highWaterMark) highWaterMark = shareValue;
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

changeRewardSpeed() will overestimate rewardsEndTimestamp and as a result, some users will receive NO/LESS rewards due to insufficient rewards provisioning. This happens because changeRewardSpeed() overestimates the amount of available reward tokens - by adding a future projected amount on top of rewardToken.balanceOf(address(this). As a result, early claimers will be able to withdraw rewards, but later claimers will not have sufficient reward tokens to withdraw (lose yield).

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Let show's how changeRewardSpeed() might cause later users lose funds by not being able to have sufficient rewards tokens to withdraw.

1. Suppose there are 360,000 rewards tokens available in the MultiRewardStaking contract for reward token X; Suppose rewardInfos[X].rewardsPerSecond = 100. Therefore, it will take another one hour for vesting all the rewards, thus rewardInfos[rewardToken].rewardsEndTimestamp = block.timestamp + 3600.

2. Now suppose the owner call changeRewardSpeed(X, 1000) to change rewardsPerSecond from 100 to 1000. Therefore, it will take only 360 seconds to vest all the tokens out. The new rewardInfos[rewardToken].rewardsEndTimestamp is supposed to be block.timestamp + 360. Unfortunately, changeRewardSpeed() will over-inflate this number.

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));

    uint256 remainder = rewardToken.balanceOf(address(this));

    uint32 prevEndTime = rewards.rewardsEndTimestamp;
    uint32 rewardsEndTimestamp = _calcRewardsEnd(
      prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
      rewardsPerSecond,
      remainder
    );
    rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
    rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp;
  }

3. Pay attention to the snippet below, we have prevEndTime = block.timestamp + 3600, and we are calling _calcRewardsEnd(block.timestamp + 3600, 1000, 360,000).

    uint32 prevEndTime = rewards.rewardsEndTimestamp;
    uint32 rewardsEndTimestamp = _calcRewardsEnd(
      prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
      rewardsPerSecond,
      remainder
    );

4. Then the _calcRewardsEnd() will return an over-inflated new rewardsEndTimestamp below. It mistakenly added 1000*3600= 3,600,000 amount on top of the balance of reward tokens, resulting 3,600,000 + 360,000 = 3, 960, 000. That is, it is trying to give 3, 960, 000 reward token out although in reality, only 360,000 tokens are available. The returned rewardsEndTimestamp is block.timestamp + 3, 960, 000/1000 = block.timestamp + 3960 instead of the correct answer block.timestamp + 360.

 function _calcRewardsEnd(
    uint32 rewardsEndTimestamp,
    uint160 rewardsPerSecond,
    uint256 amount
  ) internal returns (uint32) {
    if (rewardsEndTimestamp > block.timestamp)
      amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);

    return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
  }

5. _accrueStatic will use this overly inflated rewards.rewardsEndTimestamp to calculate accrued, which will then be used by _accurueRewards() to update reward index.

  function _accrueStatic(RewardInfo memory rewards) internal view returns (uint256 accrued) {
    uint256 elapsed;
    if (rewards.rewardsEndTimestamp > block.timestamp) {
      elapsed = block.timestamp - rewards.lastUpdatedTimestamp;
    } else if (rewards.rewardsEndTimestamp > rewards.lastUpdatedTimestamp) {
      elapsed = rewards.rewardsEndTimestamp - rewards.lastUpdatedTimestamp;
    }

    accrued = uint256(rewards.rewardsPerSecond * elapsed);
  }

function _accrueRewards(IERC20 _rewardToken, uint256 accrued) internal {
    uint256 supplyTokens = totalSupply();
    uint224 deltaIndex;
    if (supplyTokens != 0)
      deltaIndex = accrued.mulDiv(uint256(10**decimals()), supplyTokens, Math.Rounding.Down).safeCastTo224();

    rewardInfos[_rewardToken].index += deltaIndex;
    rewardInfos[_rewardToken].lastUpdatedTimestamp = block.timestamp.safeCastTo32();
  }

6. Finally, we are delivering a 3, 960, 000 tokens over 3960 seconds promise while in reality we only have 360,000 reward tokens. Early claimers who withdraw will claim more tokens than they deserve, and later claimers will claim zero or less tokens as the balance runs out (the last successful claimer will claim partially and then afterwards, all claimers will get zero).

Tools Used

Remix

Recommended Mitigation Steps

When we call _calcRewardsEnd(), we just need to pass the current time as the first argument rewardsEndTimestamp, so in this way, there is no additional amount that will be added on top of existing reward token balance.

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));

    uint256 remainder = rewardToken.balanceOf(address(this));

    uint32 prevEndTime = rewards.rewardsEndTimestamp;
-    uint32 rewardsEndTimestamp = _calcRewardsEnd(
-      prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
-      rewardsPerSecond,
-      remainder
-    );
+    uint32 rewardsEndTimestamp = _calcRewardsEnd(
+      block.timestamp.safeCastTo32(),
+     rewardsPerSecond,
+     remainder
+    );

    rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
    rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp;
  }

 function _calcRewardsEnd(
    uint32 rewardsEndTimestamp,
    uint160 rewardsPerSecond,
    uint256 amount
  ) internal returns (uint32) {
    if (rewardsEndTimestamp > block.timestamp)
      amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);

    return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
  }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

A malicious/compromised owner of MultiRewardStaking can initiate a DOS attack to _deposit(), _withdraw(), _transfer(), and _claimRewards(). As a result, any user attempts to call these functions will fail.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show how the DOS attack might happen:

1. All four functions, _deposit(), _withdraw(), _transfer(), and _claimRewards(), use the modifier accrueRewards():

 function _deposit(
    address caller,
    address receiver,
    uint256 assets,
    uint256 shares
  ) internal override accrueRewards(caller, receiver) {

function _transfer(
    address from,
    address to,
    uint256 amount
  ) internal override accrueRewards(from, to) {

 function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {

function _withdraw(
    address caller,
    address receiver,
    address owner,
    uint256 assets,
    uint256 shares
  ) internal override accrueRewards(caller, receiver) {

2. The accrueRewards() iterates through each reward token, as a result, if the list of reward token is too long, it will revert due to out of gas.

  modifier accrueRewards(address _caller, address _receiver) {
    IERC20[] memory _rewardTokens = rewardTokens;
    for (uint8 i; i < _rewardTokens.length; i++) {
      IERC20 rewardToken = _rewardTokens[i];
      RewardInfo memory rewards = rewardInfos[rewardToken];

      if (rewards.rewardsPerSecond > 0) _accrueRewards(rewardToken, _accrueStatic(rewards));
      _accrueUser(_receiver, rewardToken);

      // If a deposit/withdraw operation gets called for another user we should accrue for both of them to avoid potential issues like in the Convex-Vulnerability
      if (_receiver != _caller) _accrueUser(_caller, rewardToken);
    }
    _;
  }

3. A malicious/compromised owner can easily call addRewardToken() to explode the reward token list, rewardTokens.

function addRewardToken(
    IERC20 rewardToken,
    uint160 rewardsPerSecond,
    uint256 amount,
    bool useEscrow,
    uint192 escrowPercentage,
    uint32 escrowDuration,
    uint32 offset
  ) external onlyOwner {
    if (asset() == address(rewardToken)) revert RewardTokenCantBeStakingToken();

    RewardInfo memory rewards = rewardInfos[rewardToken];
    if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken);

    if (amount > 0) {
      if (rewardsPerSecond == 0) revert ZeroRewardsSpeed();
      rewardToken.safeTransferFrom(msg.sender, address(this), amount);
    }

    // Add the rewardToken to all existing rewardToken
    rewardTokens.push(rewardToken);

    if (useEscrow) {
      escrowInfos[rewardToken] = EscrowInfo({
        escrowPercentage: escrowPercentage,
        escrowDuration: escrowDuration,
        offset: offset
      });
      rewardToken.safeApprove(address(escrow), type(uint256).max);
    }

    uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();
    uint32 rewardsEndTimestamp = rewardsPerSecond == 0
      ? block.timestamp.safeCastTo32()
      : _calcRewardsEnd(0, rewardsPerSecond, amount);

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

    emit RewardInfoUpdate(rewardToken, rewardsPerSecond, rewardsEndTimestamp);
  }

4. As a result, all four functions, _deposit(), _withdraw(), _transfer(), and _claimRewards(), will revert due to out of gas.

Tools Used

Remix

Recommended Mitigation Steps

Suggestions:

1. Limit the maximum number of reward tokens that can be added.

2. Consider a token removal function to remove inactive/malicious bad reward tokens.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Depositors might lose funds due to the lack of checking whether the shares to be minted is equal to zero. When this happens, the assets will be deposited into the vault, but the depositors will receive ZERO shares. This is independent from the initial depositor attack issue, which means, even the first depositor attack issue is resolved, the issue here still needs to be addressed.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

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

We show how depositors might lose funds due to the lack of zero shares check.

1. Suppose currently, each share is 1000 asset tokens, and there are 1000 shares and thus 1,000,000 asset tokens in the vault. Suppose Alice call deposit() with assets = 999 as follows:

 function deposit(uint256 assets, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 shares)
    {
        if (receiver == address(0)) revert InvalidReceiver();

        uint256 feeShares = convertToShares(
            assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)
        );

        shares = convertToShares(assets) - feeShares;

        if (feeShares > 0) _mint(feeRecipient, feeShares);

        _mint(receiver, shares);

        asset.safeTransferFrom(msg.sender, address(this), assets);

        adapter.deposit(assets, address(this));

        emit Deposit(msg.sender, receiver, assets, shares);
    }

2. The number of shares will be calculated as follows, 999*1,000/1000,000 = 0.

    shares = convertToShares(assets) - feeShares;

function convertToShares(uint256 assets) public view returns (uint256) {
        uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

        return
            supply == 0
                ? assets
                : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
    }

3. Normally, when shares = 0; the deposit() function should revert, but in this case it does not. As a result, the depositor will lose 999 asset tokens and return zero shares.

4. if the price per share is inflated to 1e18/share, then a depositor can lose a maximum of 1e18-1 asset tokens while receiving zero shares!

Tools Used

Remix

Recommended Mitigation Steps

It is critical to have a zero share check and revert when the number of shares is zero.

 function deposit(uint256 assets, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 shares)
    {
        if (receiver == address(0)) revert InvalidReceiver();

        uint256 feeShares = convertToShares(
            assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)
        );

        shares = convertToShares(assets) - feeShares;

+     if(shares = 0) revert ZeroSharesDeposit();

        if (feeShares > 0) _mint(feeRecipient, feeShares);

        _mint(receiver, shares);

        asset.safeTransferFrom(msg.sender, address(this), assets);

        adapter.deposit(assets, address(this));

        emit Deposit(msg.sender, receiver, assets, shares);
    }

QA1. More topics need to be added to the emit.

a) Timestamp needs to be added to show when the fee was changed. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L544

emit ChangedFees(fees, proposedFees);

b) Timestamp needs to be added to show when the recipient was changed. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L556

emit FeeRecipientUpdated(feeRecipient, _feeRecipient);

c) Timestamp needs to be added to show when ragequit is changed.

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

emit QuitPeriodSet(quitPeriod);

d) Timestamp needs to be added to show when adaptor is changed. [https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L606(https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L606)

emit ChangedAdapter(adapter, proposedAdapter);

and more for the need of timestamp as a topic: https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L755

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

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

Needs to emit template category https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L868

QA2. Use safeApprove and safeTransfer and safeTransferFrom instead of calling directly the ERC20-standard counterparts. The safe versions use lower level calls to handle both cases from the ERC20 implementation: their functions might or might not return a value. The non-safe version does not handle both cases and ignores return values, which can leave failure undetected.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/utils/SafeERC20Upgradeable.sol

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

QA3. It is advised to lock all contract to a particular Solidity compiler version such as 0.8.10 (not too old but not too recent)

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

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

QA4. Misleading error name: https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L299

 if (rewardsPerSecond == 0) revert ZeroAmount();

Suggestions: change it to

 if (rewardsPerSecond == 0) revert ZeroRewardsPerSecond();

QA5. It is important to declare a uint _gap[50] state variable for the following upgradable implementation contracts so that when they are upgraded with the introduction of new state variables, other inheriting contracts will not be disturbed. A storage gap allows new variables to be added in future versions of the contracts without changing the inheritance chain. see https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps for further explanation.

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

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/beefy/BeefyAdapter.sol#L20

QA6. The isClaimable() fails to consider other two conditions in the _getClaimableAmount() function. We need to refactor them into isClaimable() to have the correct implementation for isClaimable():

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

- if (
-      escrow.lastUpdateTime == 0 ||
-      escrow.end == 0 ||
-      escrow.balance == 0 ||
-      block.timestamp.safeCastTo32() < escrow.start
-    ) {
+ if(!isClaimable())
      return 0;
    }

function isClaimable(bytes32 escrowId) external view returns (bool) {
-    return escrows[escrowId].lastUpdateTime != 0 && escrows[escrowId].balance > 0;
+   return escrows[escrowId].lastUpdateTime != 0 && escrows[escrowId].balance > 0 &
+              escrow.end != 0 && block.timestamp.safeCastTo32() > escrow.start();
+                
  }

QA7. Might be a good idea to change underlyingBalance to underlyingShares:

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/beefy/BeefyAdapter.sol#L110

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/beefy/BeefyAdapter.sol#L117

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/beefy/BeefyAdapter.sol#L132

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

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

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