Byzantine

Description

The CompoundV3AdapterFactory is responsible for deploying multiple instances of CompoundV3Adapters, each configured with different parentVault and comet address parameters.

After deployment, the address of each new adapter is stored in the compoundV3Adapter[parentVault][comet] mapping. However, this mapping only considers parentVault and comet as keys, and does not include the cometRewards parameter.

This creates a critical vulnerability where:

• Any user can deploy a new adapter with the same parentVault and comet, but different cometRewards
• The new deployment will overwrite the existing mapping entry
• Downstream systems relying on this mapping will be affected by the malicious overwrite

Likelihood explanation

The issue has a HIGH likelihood since it can be triggered by a random user simply by paying minimal gas costs.

Impact explanation

The contract is not directly used by other contracts within the scope of the audit. Therefore, the assumption is that any external integrators relying on the mapping could be affected, which in some cases might lead to fund loss. Due to the uncertain nature of this impact, it is assessed as LOW.

Recommendation

Consider adding validation to prevent overwrites or including cometRewards in the mapping key structure:

// Option 1: Prevent overwritesrequire(compoundV3Adapter[parentVault][comet] == address(0), "Adapter already exists");
// Option 2: Use three-dimensional mappingmapping(address => mapping(address => mapping(address => address))) public compoundV3Adapter;

Description

The CompoundV3Adapter lacks essential protection in its skim() function that prevents the withdrawal of underlying protocol tokens, while the ERC4626MerklAdapter correctly implements this safeguard.

This inconsistency creates a significant risk where the skim recipient could maliciously or accidentally withdraw the adapter's Compound V3 shares (cTokens), leading to vault insolvency.

In the CompoundV3Adapter.skim() function, there is no validation to prevent skimming of the underlying Compound V3 tokens (cTokens) that represent the adapter's allocated assets:

// CompoundV3Adapter.sol - VULNERABLEfunction skim(address token) external {    if (msg.sender != skimRecipient) revert NotAuthorized();    uint256 balance = IERC20(token).balanceOf(address(this));    SafeERC20Lib.safeTransfer(token, skimRecipient, balance);    emit Skim(token, balance);}

In contrast, the ERC4626MerklAdapter.skim() correctly implements protection:

// ERC4626MerklAdapter.sol - SECUREfunction skim(address token) external {    require(msg.sender == skimRecipient, NotAuthorized());    require(token != erc4626Vault, CannotSkimERC4626Shares()); // ✅ Protection    uint256 balance = IERC20(token).balanceOf(address(this));    SafeERC20Lib.safeTransfer(token, skimRecipient, balance);    emit Skim(token, balance);}

Likelihood explanation

The issue has a LOW likelihood since it is a protected role and the odds of this happening has slightly lower chance.

Impact explanation

Since the issue can make the parent vault insolvent, the impact from this issue is HIGH

Recommendation

Add the exact protection mechanism used in ERC4626MerklAdapter to prevent skimming of the underlying Compound V3 tokens.

Description

Both CompoundV3Adapter and ERC4626MerklAdapter lack slippage protection in their reward claiming and swapping mechanisms. The claim functions execute token swaps without minimum output validation, exposing users to potential MEV attacks, sandwich attacks, and unfavorable swap rates that could result in significant value loss during reward liquidation.

The function only checks if the receiver received at least 1 wei amount of tokens (which is highly dangerous in some instances).

Likelihood explanation

By using Flashbot RPC / other private mempool solution for claim(), this issue can be avoided to a certain degree. Provided that the problem is given a MEDIUM likelihood.

Impact explanation

Since the reward value can be significantly reduced and can be severe depending on the claim value, the issue has MEDIUM to HIGH impact.

Recommendation

Consider implementing comprehensive slippage protection by adding minimum output validation to ensure accurate results.

Byznatine

Fixed in f7c3cf5 and 3104970

Description

The adapters prevent the claim function from interacting with the underlying vaults which is critical; however, it would also be good to ensure that the claim functions do not call the parentVault as well. If they could successfully do this it would reset the approval between the adapter and the parentVault.

Recommendation

Add a require check that the swapper is not the parentVault. For extra safety it might also make sense to block calling the reward contracts in these functions.

Description

The claim() function in ERC4626MerklAdapter uses an unnecessary intermediate data structure (ClaimParams) that wraps MerklParams and SwapParams[], resulting in additional encoding/decoding overhead and increased gas consumption.

The function could be optimized by directly encoding the required parameters without the wrapper struct.

Recommendation

Consider replacing the wrapper struct with direct parameter encoding:

function claim(bytes calldata data) external {    require(msg.sender == claimer, NotAuthorized());
    // Direct decoding - OPTIMIZED    (        MerklParams memory merklParams,        SwapParams[] memory swapParams    ) = abi.decode(data, (MerklParams, SwapParams[]));
    // Claim data checks    require(swapParams.length == merklParams.tokens.length, InvalidData());
    // ... rest of function remains the same}

Description

There is less than complete test coverage of two key contracts under review (CompoundV3Adapter & ERC4626MerklAdapter).

Adequate test coverage and regular reporting are essential processes in ensuring the codebase works as intended. Insufficient code coverage may lead to unexpected issues and regressions arising due to changes in the underlying smart contract implementation

Recommendation

Add to test coverage ,ensuring all execution paths are covered.

Description

During the review, the project team brought up this issue.

On CompoundV3Adapter, the reward tokens are supposed to accrue every second. As a result, some tokens might accumulate between the generation of the claim and swap data (off-chain) and the transaction being executed on-chain. These dust tokens are locked in the adapter contractor, unless the off-chain mechanism is accurate.

To fix this issue, the adapter now approves the swapper for the entire balance of the adapter, rather than the claimed amount.

This specific issue is fixed in two commits: 14671d and 50509b

Cantina

Verified fixes. This change allows the swapper to handle donated tokens rather than having to call skim().