Popcorn contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The changeRewardSpeed() function uses the contract's token balance as the remaining tokens to distribute. But, the token balance doesn't represent the remaining balance of the reward distribution. Users who've accrued rewards don't have to have claimed their rewards at the time changeRewardSpeed() is called. The contract's balance will include their tokens as well.

Thus, using the balance as the remaining tokens to distribute for a given reward distribution won't work. The contract will try to distribute more tokens than it actually has.

Note: It has the same effect as another issue I've submitted ("MultiRewardStaking.changeRewardSpeed() breaks the distribution"). But, because the cause is different I've decided to submit them separately. It's two separate bugs resulting in the same vulnerability.

Proof of Concept

remainder is set to the contract's 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
    );
    rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
    rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp;
  }

Tools Used

none

Recommended Mitigation Steps

Calculate the remaining tokens using the rewardsPerSecond and the time that passed since the start of the distribution. Keep in mind, that rewardsPerSecond might have changed at some point.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L110-L122 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L134

Vulnerability details

Impact

This is a known issue with ERC4626 vaults. The first depositor can mint 1 share and then send a large amount, $x$, of assets to the vault directly. Subsequent depositors have to deposit more than $x$ tokens. If they don't, the funds will be deposited without them receiving any shares. The first depositor can then redeem their share to withdraw all the assets inside the vault.

Proof of Concept

1. Alice deposits 1 asset to mint 1 share
2. Alice sends a large number of assets, e.g. 100,000e18 assets to the vault
3. Bob deposits 99,000e18 assets and mints 0 shares
4. Alice withdraws 199,000e18 tokens by redeeming her share.

The deposit() rounds down without checking that shares != 0 as done in the solmate ERC4626 implementation:

    function deposit(uint256 assets, address receiver)
        public
        virtual
        override
        returns (uint256)
    {
        if (assets > maxDeposit(receiver)) revert MaxError(assets);

        uint256 shares = _previewDeposit(assets);
        _deposit(_msgSender(), receiver, assets, shares);

        return shares;
    }

The same thing applies to the Vault's deposit() function.

In case of this protocol there's an additional risk when a vault changes its adapter. It's a two-step process with a timelock of usually 3 days where the vault creator first proposes a new adapter: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L313-L344

  function proposeVaultAdapters(address[] calldata vaults, IERC4626[] calldata newAdapter) external {
    uint8 len = uint8(vaults.length);

    _verifyEqualArrayLength(len, newAdapter.length);

    ICloneRegistry _cloneRegistry = cloneRegistry;
    for (uint8 i = 0; i < len; i++) {
      _verifyCreator(vaults[i]);
      if (!_cloneRegistry.cloneExists(address(newAdapter[i]))) revert DoesntExist(address(newAdapter[i]));

      (bool success, bytes memory returnData) = adminProxy.execute(
        vaults[i],
        abi.encodeWithSelector(IVault.proposeAdapter.selector, newAdapter[i])
      );
      if (!success) revert UnderlyingError(returnData);
    }
  }

  /**
   * @notice Change adapter of a vault to the previously proposed adapter.
   * @param vaults Addresses of the vaults to change
   */
  function changeVaultAdapters(address[] calldata vaults) external {
    uint8 len = uint8(vaults.length);
    for (uint8 i = 0; i < len; i++) {
      (bool success, bytes memory returnData) = adminProxy.execute(
        vaults[i],
        abi.encodeWithSelector(IVault.changeAdapter.selector)
      );
      if (!success) revert UnderlyingError(returnData);
    }
  }

After that the timelock, the adapter is changed by first redeeming all the shares of the existing adapter. Then, it deposits all its assets into the new one: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L594

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

        adapter.redeem(
            adapter.balanceOf(address(this)),
            address(this),
            address(this)
        );

        asset.approve(address(adapter), 0);

        emit ChangedAdapter(adapter, proposedAdapter);

        adapter = proposedAdapter;

        asset.approve(address(adapter), type(uint256).max);

        adapter.deposit(asset.balanceOf(address(this)), address(this));
    }

Because of the timelock, the new adapter address will be known prior to the migration for at least a day. If there's an attacker with enough funds, they can execute the attack discussed earlier to steal all of the vault's funds.

Tools Used

none

Recommended Mitigation Steps

Check that the result of previewDeposit() is not 0 in the deposit() function.

Lines of code

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

Vulnerability details

Impact

harvest() is only supposed to be executed after the harvestCooldown is passed. But, because the contract doesn't update the lastHarvest timestamp, it will always be executed.

This will cause depositing and withdrawing funds from the adapter to be significantly more expensive on every call that the users have to pay for.

Proof of Concept

In harvest() lastHarvest is not updated. The if-clause will always evaluate to true:

    /**
     * @notice Execute Strategy and take fees.
     * @dev Delegatecall to strategy's harvest() function. All necessary data is passed via `strategyConfig`.
     * @dev Delegatecall is used to in case any logic requires the adapters address as a msg.sender. (e.g. Synthetix staking)
     */
    function harvest() public takeFees {
        if (
            address(strategy) != address(0) &&
            ((lastHarvest + harvestCooldown) < block.timestamp)
        ) {
            // solhint-disable
            address(strategy).delegatecall(
                abi.encodeWithSignature("harvest()")
            );
        }

        emit Harvested();
    }

Tools Used

none

Recommended Mitigation Steps

Add lastHarvest = block.timestamp after the if-block.

Lines of code

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

Vulnerability details

Impact

The changeRewardSpeed() doesn't calculate the new endTimestamp correctly. That causes the reward distribution to be broken

Proof of Concept

Given that we have an existing reward with the following configuration:

• startTimestamp = 0
• endTimestamp = 100
• rewardPerSecond = 2
• initialBalance = 200

The reward speed is changed at timestamp = 50, meaning 100 tokens were already distributed. The new endTimestamp is calculated by calling _calcRewardsEnd():

    // @audit using balanceOf() here has its own issues but let's ignore those for this submission
    uint256 remainder = rewardToken.balanceOf(address(this));

    uint32 prevEndTime = rewards.rewardsEndTimestamp;

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

And the calculation is:

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

• rewardsEndTimestamp = 100 (initial endTimestamp)
• block.timestamp = 50 (as described earlier)
• amount = 100
• rewardsPerSecond = 4 (we update it by calling this function)

Because rewardEndTimestamp > block.timestamp, the if clause is executed and amount is increased: $amountNew = 100 + 4 * (100 - 50) = 300$

Then it calculates the new endTimestamp: $50 + (300 / 4) = 125$

Thus, by increasing the rewardsPerSecond from 2 to 4, we've increased the endTimestamp from 100 to 125 instead of descreasing it. The total amount of rewards that are distributed are calculated using the rewardsPerSecond and endTimestamp. Meaning, the contract will also try to distribute tokens it doesn't hold. It only has the remaining 100 tokens.

By increasing the rewardsPerSecond the whole distribution is broken.

Tools Used

none

Recommended Mitigation Steps

It's not easy to fix this issue with the current implementation of the contract. There are a number of other issues. But, in essence:

• determine the remaining amount of tokens that need to be distributed
• calculate the new endTimestamp: endTimestamp = remainingAmount / newRewardsPerSecond

Lines of code

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

Vulnerability details

Impact

The vault's quitPeriod can be updated through the setQuitPeriod() function. Only the owner of the contract (AdminProxy through VaultController) can call it. But, the VaultController contract doesn't implement a function to call setQuitPeriod(). Thus, the function is actually not usable.

This limits the configuration of the vault. Every vault will have to use the standard 3-day quitPeriod.

Proof of Concept

setQuitPeriod() has the onlyOwner modifier which only allows the AdminProxy to access the function. The AdminProxy is called through the VaultController.

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

        quitPeriod = _quitPeriod;

        emit QuitPeriodSet(quitPeriod);
    }

The VaultController doesn't provide a function to execute setQuitPeriod(). Just search for setQuidPeriod.selector and you won't find anything.

Tools Used

none

Recommended Mitigation Steps

Add a function to interact with setQuitPeriod().

Lines of code

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

Vulnerability details

Impact

The vault's feeRecipient can be updated through the setFeeRecipient() function. Only the owner of the contract (VaultController) can call it. But, the VaultController contract doesn't implement a function to call setFeeRecipient(). Thus, the function is actually not usable.

Since the vault creator won't be able to change the fee recipient they might potentially lose access to those funds.

Proof of Concept

setFeeRecipient has the onlyOwner modifier which only allows the AdminProxy to access the function. The AdminProxy is called through the VaultController.

    function setFeeRecipient(address _feeRecipient) external onlyOwner {
        if (_feeRecipient == address(0)) revert InvalidFeeRecipient();

        emit FeeRecipientUpdated(feeRecipient, _feeRecipient);

        feeRecipient = _feeRecipient;
    }

The VaultController doesn't provide a function to execute setFeeRecipient(). Just search for setFeeRecipient.selector and you won't find anything.

Tools Used

none

Recommended Mitigation Steps

Add a function to interact with setFeeRecipient().

Lines of code

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

Vulnerability details

Impact

Vault.changeFees() sets the vault's fees to the previously set proposedFees. The function is callable by anyone. After the vault is created, proposedFees is set to its zero-value. Thus, the attacker is able to call changeFees() to set all the vault's fees to 0. For the vault owner to rewind those changes, they have to propose new fees which comes with a waiting period of 3 days by default.

Thus, an attacker is able to cause the vault owner to lose 3 days' worth of revenue.

Proof of Concept

changeFees() is callable by anyone. After deployment, proposedFeeTime and proposedFees are both set to their zero-value. Thus, the if clause will not be executed.

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

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

The vault owner has to propose new fees through the VaultController: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L352

For the new fees to take effect, the quitPeriod in changeFees() has to pass which is set to 3 days by default: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L619

Tools Used

none

Recommended Mitigation Steps

Add following check to changeFees:

if (proposedFeeTime == 0) revert NoFeesProposed();

Report

01: MultiRewardEscrow.isClaimable() always returns true

  function isClaimable(bytes32 escrowId) external view returns (bool) {
    return escrows[escrowId].lastUpdateTime != 0 && escrows[escrowId].balance > 0;
  }

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

When an escrow is created both lastUpdateTime and balance will be != 0. But, that doesn't mean that the escrow is claimable, e.g. it might not have started yet.

02: Vault's fees are not validated on initialization

When new fees are proposed for a vault they are validated to be <= 1e18. That doesn't happen in the initialize() function. The vault creator could shoot themselves in the foot by setting the wrong fees.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L88 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L526

03: Vault's previewMint() and previewWithdraw() take more fees than specified

The calculation is wrong. Instead of assets * fee / (1e18 - fee) it should be assets * fee / 1e18 to determine the fee.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L345 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L367