Popcorn contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

MultiRewardStaking.changeRewardSpeed() calculates the rewardsEndTimestamp longer than real value.

As a result, rewards for every user will be inflated and some users won't claim their rewards because the contract doesn't have enough balance.

Proof of Concept

changeRewardSpeed() is used to change the rewardsPerSecond by admin.

  function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner {
    ...

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

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

Currently, it uses a remainder amount(existing reward token balance inside the contract) for the calculation but this amount shouldn't be used as it contains an unclaimed rewards also.

As a result, rewardsEndTimestamp will be calculated larger than it should and rewards for users will be accrued larger in _accrueStatic().

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

So the total amounts of rewards to pay will be more than the actual balance of reward token inside the contract and some users can't claim their rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

In changeRewardSpeed(), we should calculate the new end time using an unpaid rewards only like below.

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

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

Lines of code

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

Vulnerability details

Impact

Users would claim rewards several times if the reward token is an ERC777 one.

Proof of Concept

MultiRewardStaking.claimRewards() resets accruedRewards after transferring funds.

  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); //@audit reentrancy
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount); //@audit reentrancy
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

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

So users can call this function several times if the reward token has a hook.

In this case, users can drain all rewards from the protocol.

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify like the below.

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

      accruedRewards[user][_rewardTokens[i]] = 0; //++++++++++++++++++++++

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

    }
  }

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/2d373f8748a140f5d7a57e738172750811449c04/src/utils/MultiRewardStaking.sol#L127

Vulnerability details

Impact

MultiRewardStaking._withdraw() doesn't track the user's rewards correctly when caller != owner.

As a result, users will lose their rewards when they withdraw their balances using other accounts.

Proof of Concept

MultiRewardStaking._withdraw() tracks the rewards using an accrueRewards modifier.

  function _withdraw(
    address caller,
    address receiver,
    address owner,
    uint256 assets,
    uint256 shares
  ) internal override accrueRewards(caller, receiver) { //@audit should update owner instead of caller
    if (caller != owner) _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);

    _burn(owner, shares);
    IERC20(asset()).safeTransfer(receiver, assets);

    emit Withdraw(caller, receiver, owner, assets, shares);
  }

But the accrueRewards modifier updates a caller instead of owner and the below scenario would be possible.

1. A user deposited a certain amount of assets and received 100 shares.
2. 1 month later, he withdrew all shares using another account.
3. At that time, the rewards state of caller(another account) and receiver were updated but not his original account.
4. After that, when he tries to claim his rewards, his rewards will be 0 because his balance is 0 already and there are no accumulated rewards.

  function _accrueUser(address _user, IERC20 _rewardToken) internal {
    ...

    uint256 deltaIndex = rewards.index - oldIndex;

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

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

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

As a result, he receives 0 rewards although he staked 100 shares for 1 month.

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify like the below.

  function _withdraw(
    address caller,
    address receiver,
    address owner,
    uint256 assets,
    uint256 shares
  ) internal override accrueRewards(owner, receiver) { //+++++++++++++++++
    if (caller != owner) _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);

    _burn(owner, shares);
    IERC20(asset()).safeTransfer(receiver, assets);

    emit Withdraw(caller, receiver, owner, assets, shares);
  }

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/yearn/YearnAdapter.sol#L109-L111

Vulnerability details

Impact

Total assets of yearn vault are not correct so the calculation between the asset and the shares will be wrong.

Proof of Concept

In YearnAdapter the total assets of current yValut are extracted using _yTotalAssets.

    function _yTotalAssets() internal view returns (uint256) {
        return IERC20(asset()).balanceOf(address(yVault)) + yVault.totalDebt(); //@audit
    }

But in the yearn vault implementation, self.totalIdle is used instead of current balance.

def _totalAssets() -> uint256:
    # See note on `totalAssets()`.
    return self.totalIdle + self.totalDebt

In yearn valut, totalIdle is the tracked value of tokens, so it is same as vault's balance in most cases, but the balance can be larger due to an attack or other's fault. Even sweep is implemented for the case in the vault implementation.

    if token == self.token.address:
        value = self.token.balanceOf(self) - self.totalIdle

    log Sweep(token, value)
    self.erc20_safe_transfer(token, self.governance, value)

So the result of _yTotalAssets can be inflated than the correct total assets and the calculation between the asset and the shares will be incorrect.

Tools Used

Manual Review

Recommended Mitigation Steps

Use yVault.totalIdle instead of balance.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/2d373f8748a140f5d7a57e738172750811449c04/src/utils/MultiRewardStaking.sol#L245

Vulnerability details

Impact

The raw rewardsPerSecond might be too big for some ERC20 tokens and it wouldn't work as intended.

Proof of Concept

As we can see from _accrueStatic(), the rewardsPerSecond is a raw amount without any multiplier.

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

But 1 wei for 1 second would be too big amount for some popular tokens like WBTC(8 decimals) and EURS(2 decimals).

For WBTC, it will be 0.31536 WBTC per year (worth about $7,200) to meet this requirement, and for EURS, it must be at least 315,360 EURS per year (worth about $315,000).

Such amounts might be too big as rewards and the protocol wouldn't work properly for such tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend introducing a RATE_DECIMALS_MULTIPLIER = 10**9(example) to increase the precision of rewardsPerSecond instead of using the raw amount.

Lines of code

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

Vulnerability details

Impact

YearnAdapter.pause can revert and pause will not work in an emergency

Proof of Concept

When YearnAdapter should be paused, all funds are withdrawn first and then _pause is called.

    function pause() external onlyOwner {
        _protocolWithdraw(totalAssets(), totalSupply());
        // Update the underlying balance to prevent inflation attacks
        underlyingBalance = 0;
        _pause();
    }

YearnAdapter._protocolWithdraw calls yVault.withdraw directly with asset amount only. But in the yValut.withdraw there is another maxLoss parameter.

def withdraw(
    maxShares: uint256 = MAX_UINT256,
    recipient: address = msg.sender,
    maxLoss: uint256 = 1,  # 0.01% [BPS]
) -> uint256:

pause can be called in an emergency and this withdraw amount can be huge. In this case yearn vault balance is not enough and it needs withdraw from strategies in withdraw queue.

if value > vault_balance:
...
    for strategy in self.withdrawalQueue:
    ...
        loss: uint256 = Strategy(strategy).withdraw(amountNeeded)
    ...
    assert totalLoss <= maxLoss * (value + totalLoss) / MAX_BPS

After it withdraws from strategies, it checks totalLoss using maxLoss. This withdraw can revert for the default maxLoss parameter and it will prevent pause in emergency.

Tools Used

Manual Review

Recommended Mitigation Steps

Add maxLoss parameter in YearnAdapter._protocolWithdraw in line 171

Lines of code

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

Vulnerability details

Impact

lastHarvest is not updated so the cooldown logic does not work.

Proof of Concept

Cooldown functionality is implemented in AdapterBase.harvest. harvest can't be run after cooltime from the last harvest time.

    function harvest() public takeFees {
        if (
            address(strategy) != address(0) &&
            ((lastHarvest + harvestCooldown) < block.timestamp)
        ) {

The cooldown check is correct but lastHarvest is not updated after harvest in the harvest function implementation. As a result lastHarvest is not changed from the initial value so the cooldown logic does not work.

Tools Used

Manual Review

Recommended Mitigation Steps

Set lastHarvest = block.timestamp after harvest() is called by delegatecall on line 444.

Lines of code

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

Vulnerability details

Impact

This modifier will affect the fee calculation.

Proof of Concept

Vault.redeem don't have syncFeeCheckpoint modifier. So highWaterMark is not updated when redeem is called.

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

And accruedPerformanceFee will be wrong as a result.

Tools Used

Manual Review

Recommended Mitigation Steps

Add syncFeeCheckpoint modifier in Vault.redeem

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/2d373f8748a140f5d7a57e738172750811449c04/src/vault/Vault.sol#L594

Vulnerability details

Impact

A malicious user can make the Vault pay the fees unnecessarily using Vault.changeAdapter().

Proof of Concept

The main reason for this issue is that any users can call changeAdapter() again and again and make the vault pay fees unnecessarily.

The admin can change the adapter using proposeAdapter() and changeAdapter().

    function proposeAdapter(IERC4626 newAdapter) external onlyOwner {
        if (newAdapter.asset() != address(asset))
            revert VaultAssetMismatchNewAdapterAsset();

        proposedAdapter = newAdapter;
        proposedAdapterTime = block.timestamp;

        emit NewAdapterProposed(newAdapter, block.timestamp);
    }

    /**
     * @notice Set a new Adapter for this Vault after the quit period has passed.
     * @dev This migration function will remove all assets from the old Vault and move them into the new vault
     * @dev Additionally it will zero old allowances and set new ones
     * @dev Last we update HWM and assetsCheckpoint for fees to make sure they adjust to the new adapter 
     */
    function changeAdapter() external takeFees { //@audit lose fee again by anyone
        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));
    }

In changeAdapter(), it redeems all amounts from the old adapter and deposits to the new adapter.

And as we can see from BeefyAdapter.previewRedeem(), the adapters charge some fees during deposit/redeem.

    function previewRedeem(uint256 shares)
        public
        view
        override
        returns (uint256)
    {
        uint256 assets = _convertToAssets(shares, Math.Rounding.Down);

        IBeefyStrat strat = IBeefyStrat(beefyVault.strategy());
        uint256 beefyFee = strat.withdrawalFee();
        if (beefyFee > 0)
            assets -= assets.mulDiv(
                beefyFee,
                BPS_DENOMINATOR,
                Math.Rounding.Up
            );

        return assets;
    }

So the below scenario would be possible.

1. In Vault.sol, the admin proposed a new adapter and changed it after quitPeriod.
2. After that, a malicious user calls changeAdapter() again.
3. As proposedAdapter and proposedAdapterTime weren't reset after the admin call and it will work properly.
4. As a result, it will redeem from the current adapter and deposit to the same one again and the vault will pay certain amounts of fees.

The malicious user can call this function as many times as he likes and it should be fixed even though there is no benefit to the attacker.

Tools Used

Manual Review

Recommended Mitigation Steps

We should reset proposedAdapter after calling changeAdapter() and make the function revert if proposedAdapter = address(0).

    function changeAdapter() external takeFees { 
        if (block.timestamp < proposedAdapterTime + quitPeriod || proposedAdapter == address(0)) //++++++++++++++++
            revert NotPassedQuitPeriod(quitPeriod);

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

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

        emit ChangedAdapter(adapter, proposedAdapter);

        adapter = proposedAdapter;

        proposedAdapter = address(0); //+++++++++++++++++++++

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

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

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/2d373f8748a140f5d7a57e738172750811449c04/src/utils/MultiRewardStaking.sol#L200

Vulnerability details

Impact

MultiRewardStaking.claimRewards() will revert when escrowPercentage = 100% and the reward token is a Revert on Zero Value Transfers one.

In this case, users can't claim their rewards.

Proof of Concept

When users claim their rewards, claimRewards function locks some percent in an escrow.

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

And _lockToken() transfers the remaining amount to the user after locking a escrowPercentage of rewards.

  function _lockToken(
    address user,
    IERC20 rewardToken,
    uint256 rewardAmount,
    EscrowInfo memory escrowInfo
  ) internal {
    uint256 escrowed = rewardAmount.mulDiv(uint256(escrowInfo.escrowPercentage), 1e18, Math.Rounding.Down);
    uint256 payout = rewardAmount - escrowed;

    rewardToken.safeTransfer(user, payout); //@audit 0 transfer
    escrow.lock(rewardToken, user, escrowed, escrowInfo.escrowDuration, escrowInfo.offset);
  }

But there is no requirement of escrowInfo.escrowPercentage < 100% and payout will be 0 when the escrowPercentage is 100%.

In this case, this function will revert if the reward token is a Revert on Zero Value Transfers and users can't claim their rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify _lockToken() like below.

  function _lockToken(
    address user,
    IERC20 rewardToken,
    uint256 rewardAmount,
    EscrowInfo memory escrowInfo
  ) internal {
    uint256 escrowed = rewardAmount.mulDiv(uint256(escrowInfo.escrowPercentage), 1e18, Math.Rounding.Down);
    uint256 payout = rewardAmount - escrowed;

    if(payout > 0) { //+++++++++++++++++++++
        rewardToken.safeTransfer(user, payout);
    }
    escrow.lock(rewardToken, user, escrowed, escrowInfo.escrowDuration, escrowInfo.offset);
  }

Lines of code

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

Vulnerability details

Impact

Vault.fees can be set to 0 by any user.

Proof of Concept

The main reason for this issue is that changeFees() works properly when proposedFeeTime = 0.

The fees can be changed by admin using proposeFees() and changeFees().

    function proposeFees(VaultFees calldata newFees) external onlyOwner {
        if (
            newFees.deposit >= 1e18 ||
            newFees.withdrawal >= 1e18 ||
            newFees.management >= 1e18 ||
            newFees.performance >= 1e18
        ) revert InvalidVaultFees();

        proposedFees = newFees;
        proposedFeeTime = block.timestamp;

        emit NewFeesProposed(newFees, block.timestamp);
    }

    /// @notice Change fees to the previously proposed fees after the quit period has passed.
    function changeFees() external { //@audit can set fees = 0
        if (block.timestamp < proposedFeeTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);

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

And changeFees() can be called by anyone even though the admin didn't propose new fee settings.

So users can set fees = 0(empty struct) by calling the function and the protocol can't charge fees for a while(at least 1 day because quitPeriod >= 1 days).

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify changeFees() to revert when proposedFeeTime = 0.

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

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

[L-01] externalRegistry is not validated in YearnAdapter.initialize

• https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/yearn/YearnAdapter.sol#L45

        yVault = VaultAPI(IYearnRegistry(externalRegistry).latestVault(_asset)); //@audit

When YearnAdapter is initialized, yVault is from externalRegistry. But there is no endorsement check about externalRegistry. So the creator of the adapter can send an invalid Yearn registry. So it is recommended to validate endorsement of externalRegistry to prevent this.

[L-02] MultiRewardEscrow.lock can revert for small fee percents

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

    uint256 feePerc = fees[token].feePerc;
    if (feePerc > 0) {
      uint256 fee = Math.mulDiv(amount, feePerc, 1e18);

      amount -= fee;
      token.safeTransfer(feeRecipient, fee);
    }

When the fee percent is very small, amount * feePerc can be less than 1e18 and fee will be 0. The transfer of zero amount will revert for some tokens and this will cause the revert of MultiRewardEscrow.lock. It is recommended to use another if conditional before the safeTransfer.

[L-03] The return type of VaultRegistry.getSubmitter is wrong

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

VaultRegistry.getSubmitter returns VaultMetadata in the implementation.

function getSubmitter(address vault) external view returns (VaultMetadata memory) {

But it should return address as we can see in the interface.

function getSubmitter(address vault) external view returns (address);

I think the return type and the implementation of VaultRegistry.getSubmitter are not correct. getSubmitter is not used in the repository so I can not check anymore.

[L-04] Large decimal tokens can't be used as reward tokens.

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

MultiRewardStaking.addRewardToken will revert for decimals > 19 in the following line.

uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();

So we can't use those tokens as reward tokens.

[N-01] Typo

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

Mismatch is not correct here.

if (length1 != length2) revert ArrayLengthMissmatch();

[N-02] Open TODOs

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

There is an open TODO for repository for an audit.