Popcorn contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

On changing adapter address, all funds are first withdrawn from old adapter and then reinvested in new adaptor. In case of some adaptor like BeefyAdapter, an extra fees (strat.withdrawalFee()) is also deducted on withdrawal. It was observed that proposedAdapter is never reset meaning user can call changeAdapter multiple times causing protocol to loss funds as withdrawal fees

Proof of Concept

1. Assume owner has proposed a new BeefyAdapter for Vault V using the proposeAdapter function

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

        proposedAdapter = newAdapter;
        proposedAdapterTime = block.timestamp;

        emit NewAdapterProposed(newAdapter, block.timestamp);
    }

2. Post proposedAdapterTime owner calls changeAdapter which changes adapter to BeefyAdapter

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

3. Attacker A calls changeAdapter function again

4. Since proposedAdapter and proposedAdapterTime are not reset, this works

//Pass since adapterTime was not reset
if (block.timestamp < proposedAdapterTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);

5. Now this redeems BeefyAdapter funds which also deducts a Beefy strategy withdrawal fees (due to bug described in separate issue, fees is not deducted currently but once fixed this will occur)

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

6. Due to withdrawal fees, not full amount is redeposited and the fees is then distributed to network users. Even though there was no change in adapter

7. Repeating these steps will cause growing losses

Recommended Mitigation Steps

Revise the changeAdapter function to change adaptor only if provided is different from existing adapter

function changeAdapter() external takeFees {
...
require(proposedAdapter!=adapter, "Same adapter");
...
}

Lines of code

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

Vulnerability details

Impact

If you are making a Lock fund for escrow using a fee on transfer token then contract will receive less amount (X-fees) but will record full amount (X). This becomes a problem as when claim is made then call will fail due to lack of funds. Worse, one user will unknowingly take the missing fees part from another user deposited escrow fund

Proof of Concept

1. User locks token X as escrow which take fee on transfer
2. For same, he uses lock function which transfer funds from user to contract

 function lock(
    IERC20 token,
    address account,
    uint256 amount,
    uint32 duration,
    uint32 offset
  ) external {
...
 token.safeTransferFrom(msg.sender, address(this), amount);
...
escrows[id] = Escrow({
      token: token,
      start: start,
      end: start + duration,
      lastUpdateTime: start,
      initialBalance: amount,
      balance: amount,
      account: account
    });
...
}

3. Since token has fee on transfer so contract receives only amount-fees but the escrow object is created for full amount

4. Lets say escrow duration is over and claim is made using claimRewards function

function claimRewards(bytes32[] memory escrowIds) external {
...
 uint256 claimable = _getClaimableAmount(escrow);
...    
 escrow.token.safeTransfer(escrow.account, claimable);
...
}

5. Since full duration is over so claimable amount is amount. But this fails on transfer to account since contract has only amount-fees

Recommended Mitigation Steps

Compute the balance before and after transfer and subtract them to get the real amount. Also use nonReentrant while using this to prevent from reentrancy in ERC777 tokens

Owner can accidentally add duplicate Clone

Contract: https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/CloneRegistry.sol#L41

Issue: On adding a duplicate entry via addClone function, multiple clone will be written on clones and allClones object.

Impact: If any future function relies directly on number of clones, or iterating allClones then it will encounter duplicate entry. if its not operated correctly then could result in incorrect outcome

Recommendation: Add below condition:

require(!cloneExists[clone], "Already exists");