Reach out for an audit or to learn more about Macro
or Message on Telegram

Sommelier A-16b

Security Audit

February 12, 2024

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Sommelier's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from January 23, 2024 to January 31, 2024.

The purpose of this audit is to review the source code of certain Sommelier Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
High 4 - 1 3
Medium 2 - 1 1
Low 2 - 2 -
Code Quality 8 - 2 6
Gas Optimization 1 - - 1

Sommelier was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Trust Model, Assumptions, and Accepted Risks (TMAAR)

Trusted entities:

The goal of the system is to to have checks and balances for each permissioned action, where if any one permissioned entity acts malicious, the others can remedy the situation, requiring multiple points failure before it can negatively impact users.

Assumptions:

Accepted Risks:

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within this repository.

Contract SHA256
src/modules/multi-chain-share/DestinationMinter.sol

a5255faff5625e3337f8785e0d0c19f37cf2a257c800e436bea99f88b593c931

src/modules/multi-chain-share/DestinationMinterFactory.sol

b155089909314f80f4194370ce309498ecf8081b937312b2f03c440351c62f7b

src/modules/multi-chain-share/SourceLocker.sol

068a70bb5a40e40bed28a322753ed277f8034d8bb569ed902b3a13e15a1a1f5b

src/modules/multi-chain-share/SourceLockerFactory.sol

9358b68d48d6ef795c668a1c82577f81ab185d41e8992da7f10ddf8ad880f82a

src/modules/adaptors/Compound/V3/BorrowAdaptor.sol

586199e16fbd3f6917fab98e8c679a6db7d55f8f61e735787b9b6828f72cbfd9

src/modules/adaptors/Compound/V3/CollateralAdaptor.sol

21835cbc1a69ef9d0212c827760c05cc7bbc0283a50a4ff912425fe33e7d526b

src/modules/adaptors/Compound/V3/RewardsAdaptor.sol

b22d669dfbbf0cd16d43dfad8cf990413d03351c079e4051b26bc3ada2071ffc

src/modules/adaptors/Compound/V3/SupplyAdaptor.sol

7c7b17c7d32924536c000f7967aea331437d73aa9f59e339acf04ba611fda8b9

src/modules/adaptors/Compound/V3/V3Helper.sol

d6870250073d76ffc9aed722cbba301af363ddeb54b4dff2d705ac34fbdf52d8

src/modules/adaptors/Staking/EtherFiStakingAdaptor.sol

a566409152358b67570b4beffdcf7c0bc84b9fcbffd65b42ac0a52d18f818624

src/modules/adaptors/Staking/KelpDAOStakingAdaptor.sol

c56bd3bb2ba81044114f86b84057aa02c098d45488cf2f60dafc4e9fd46d1a64

src/modules/adaptors/Staking/LidoStakingAdaptor.sol

5a36ec0065cb9aea5960b3a213096b60b7cb462c7130330839911947b2dbc261

src/modules/adaptors/Staking/RenzoStakingAdaptor.sol

19068b6b2a9e9c4bc4d4773e4d7409306e8d25e881c3242ccf99dff18caf4021

src/modules/adaptors/Staking/StaderStakingAdaptor.sol

b0eebb1502f4fdb4e262f2c5e7db4a4689ffed8ffa858f065ae6629afe9fc904

src/modules/adaptors/Staking/StakingAdaptor.sol

32d872bc07cbe94aeb86b4f08ca6950dce99d966b626812fe58b127b4583725b

src/modules/adaptors/Staking/SwellStakingAdaptor.sol

5ca190207fa2fb1bc24f1363ab8b351ee436cab64863d6394a2220425bc4f4be

src/modules/price-router/Extensions/EtherFi/eETHExtension.sol

41dd439ff7f7f7ce0ffb91fe5d00dbbf5f16ad4dab720600832be549aafc42c8

src/modules/atomic-queue/AtomicQueue.sol

4349ae3bb0f61f5d8123718ee66adc115f0eb980b681f4765039c71f615e3f63

Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

H-1

Chainlink could change the router address

Topic
Error Recovery
Status
Wont Do
Impact
Critical
Likelihood
Low

Recently chainlink changed their ccip router address, and will depreciate the old router, no longer supporting it after March 31st 2024. Currently, contracts using ccip directly interact with the router and it is set to be immutable, assuming it will not change. If chainlink does change the router in the future, tokens bridged may not be able to be bridged back and recovered, removing the backing of the tokens created in the DestinationMinter contract, and locking them in the SourceLocker contract forever.

Remediations to Consider

Assuming a new router would be backwards compatible with the current one, allowing the owner to update to the new router would allow contracts to adapt to a router change. Consider adding a timelock to the contract,

Response by Sommelier

Multichain products are very experimental, and it is difficult to foresee all the ways a multichain design could fail. With this in mind we are choosing to keep the current design as simple as possible, and without owner permissions, so that it is as flexible as possible to unknown issues. That being said, if the router address does change, we understand we will need to inform all users to bridge their shares back to the source chain, so that they are not lost forever. We can then deploy a new set of multichain contracts with the updated router address.

H-2

Lido withdrawal amount may be less than assumed

Topic
Incentive Design
Impact
High
Likelihood
Low

In LidoStakingAdaptor.sol's balanceOf() function it is assumed that the amount of stETH deposited in a request will be equal to the amount of ETH received when the request is finalized, or in other words that 1 stETH will be exchanged for 1 ETH:

function _balanceOf(address account) internal view override returns (uint256 amount) {
    uint256[] memory requests = StakingAdaptor(adaptorAddress).getRequestIds(account);
    IUNSTETH.WithdrawalRequestStatus[] memory statuses = unstETH.getWithdrawalStatus(requests);
    for (uint256 i; i < statuses.length; ++i) {
        amount += statuses[i].amountOfStETH;
    }
}

Reference: LidoStakingAdaptor.sol#L94-L99

However, when a request is finalized, a max rate is set that limits the amount of ETH received to be either nominal: 1 stETH = 1 ETH or at a discount, where less ETH is received than stETH deposited, as mentioned in the withdrawalQueue comments can occur if the protocol share rate drops:

//
// FINALIZATION FLOW
//
// Process when protocol is fixing the withdrawal request value and lock the required amount of ETH.
// The value of a request after finalization can be:
//  - nominal (when the amount of eth locked for this request are equal to the request's stETH)
//  - discounted (when the amount of eth will be lower, because the protocol share rate dropped

Reference: WithdrawalQueueBase.sol#L148-154

In cases where a finalized request end up with a discounted rate, then LidoStakingAdaptor's balanceOf() will report a larger value of ETH owed than it actually would receive when withdrawn. This would cause an inaccurate share price, and a share price drop when the ETH is withdrawn on rebalance, potentially preventing a withdrawal if the loss exceeds the rebalance deviation.

It is important to note that for unfinalized requests it is not possible to estimate the expected ETH that will be claimable once finalized, so it has to be assumed it can be exchanged at a 1:1 rate, which may not be accurate if the exchange rate decreases. There may be conditions that _balanceOf() would report inaccurate values for unfinalized requests, which could lead to a drop in share value once these requests get finalized, which may give users an opportunity to react to this delay.

Remediations to Consider

In balanceOf(), use the same pricing for the unfinalized requests, but for the requests that are finalized, call getClaimableEther() for each request after querying the request id’s hint using findCheckpointHints(). This will ensure an accurate balanceOf() for finalized requests will be calculated and properly cover large deviations to withdrawal rates.

This solution is quite gas intensive, and as mentioned in H-4 it could revert due to out of gas errors due to the unbounded loops made to calculate the hint in findCheckpointHints(). Consideration should be made, potentially limiting the maximum withdrawal requests.

Response by Sommelier

Re stETH ETH 1:1 assumed exchange rate. Under 99.99% of market conditions, this is a safe assumption, that Sommelier has made for months with pricing stETH, along with multiple other protocols. With this in mind, there are certainly black swan events that can make this assumption problematic, however Cellars using this adaptor will be Share Price Oracle based Cellars, which greatly reduce the arbability of them when it comes to single TXs altering the Cellar’s share price, which greatly mitigates the downsides of this assumption.

H-3

Stader withdrawal amount may be less than assumed

Topic
Incentive Design
Status
Impact
High
Likelihood
Medium

In StaderStakingAdaptor’s balanceOf() function, the balance is calculated by iterating over the users withdrawal requests and accumulating the amount based on the current exchange rate.

for (uint256 i; i < requestsLength; ++i) {
    IUserWithdrawManager.WithdrawRequest memory request = userWithdrawManager.userWithdrawRequests(
        uint256(requests[i])
    );
    uint256 ethXValueUsingCurrentExchangeRate = request.ethXAmount.mulDivDown(exchangeRate, DECIMALS);
    amount += request.ethExpected.min(ethXValueUsingCurrentExchangeRate);
}

Reference: StaderStakingAdaptor.sol#L75-L88

The above calculation doesn’t explicitly account for the finalized withdrawal requests where the amount was calculated with an older exchange rate and is tracked in the WithdrawRequest.ethFinalized property.

Under normal market conditions, the exchange rate is expected to increase over time. Thus, in cases where finalized withdrawal requests that have not been claimed yet, the balanceOf function reports a larger value than the actual amount that can be withdrawn.

Similar to [H-1], this would cause an inaccurate share price, and a share price drop when the ETH is withdrawn on rebalance, potentially preventing a withdrawal if the loss exceeds the rebalance deviation.

Remediations to Consider

Adjust balanceOf function to use ethFinalized value for finalized request (this is the case when ethFinalized > 0)

H-4

Lido withdrawals may revert

Topic
Incentive design
Status
Impact
High
Likelihood
Low

LidoStakingAdaptor’s _completeBurn function uses WithdrawalQueue.claimWithdrawal function to redeem assets from Lido:

function _completeBurn(uint256 id) internal override {
    unstETH.claimWithdrawal(id);
}

Reference: LidoStakingAdaptor.sol#L124

As mentioned in the claimWithdrawal comments, this function is susceptible to run out-of-gas due to the use of an unbounded loop:

/// @dev use unbounded loop to find a hint, which can lead to OOG

Reference: WithdrawalQueue.sol#L279

This can prevent strategists to withdraw assets from the LidoStakingAdaptor.

According to Lido’s documentation, it is recommended to provide a hint value to optimize the claiming flow and to avoid running out-of-gas.

Remediations to Consider

Provide the option to specify a hint value (which can be queried using findCheckpointHints()) for the completeBurn function, so that strategist can opt for the optimized claiming flow or to avoid running out-of-gas.

M-1

Potential incompatibility with future CCIP upgrades

Topic
Input Validation
Status
Wont Do
Impact
High
Likelihood
Medium

According to the Chainlink CCIP Best Practices, it is recommended to make the extraArgs value mutable:

The purpose of extraArgs is to allow compatibility with future CCIP upgrades. To get this benefit, make sure that extraArgs is mutable in production deployments.

In the current implementation, the extraArgs value is computed as follows:

extraArgs: Client._argsToBytes(
    // Additional arguments, setting gas limit and non-strict sequencing mode
    Client.EVMExtraArgsV1({ gasLimit: messageGasLimit /*, strict: false*/ })
),

Since the extraArgs value is hardcoded and cannot be altered after deployment, it may break compatibility with future CCIP upgrades.

Remediations to Consider

Consider computing the extraArgs value off-chain and providing it via an owner-controlled setter function; or alternatively allow users to set the value when bridging the shares.

Response by Sommelier

As mentioned in H-1 we are prioritizing simplicity and flexibility, and accepting the fact that if Chainlink makes breaking changes in the future we will need to deprecate and re-deploy

M-2

Withdrawal Requests completion book keeping could be skipped

Topic
Error Recovery
Impact
Medium
Likelihood
Low

Staking Adaptors that inherit StakingAdaptor.sol assume that only the cellar can complete a pending withdrawal request. This is important since an array of requestIds is added to and removed on request and completion respectively, as well as any ETH sent to the cellar on withdrawal is immediately wrapped so it can be properly handled and tracked by the cellar.

/**
 * @notice Allows a strategist to request to burn/withdraw a derivative for a chains native asset.
 * @param amount the amount of derivative to burn/withdraw
 */
function requestBurn(uint256 amount) external virtual {
    if (amount == 0) revert StakingAdaptor__ZeroAmount();

    uint256 id = _requestBurn(amount);

    // Add request id to staking adaptor.
    StakingAdaptor(adaptorAddress).addRequestId(id);
}

/**
 * @notice Allows a strategist to complete a burn/withdraw of a derivative asset for a native asset.
 * @dev Will automatically wrap the native asset received from burn/withdraw.
 * @param id the request id
 */
function completeBurn(uint256 id) external virtual {
    uint256 primitiveDelta = address(this).balance;
    _completeBurn(id);
    primitiveDelta = address(this).balance - primitiveDelta;
    wrappedPrimitive.deposit{ value: primitiveDelta }();
    StakingAdaptor(adaptorAddress).removeRequestId(id);
}

Reference: StakingAdaptor.sol#L232-L256

However, if a protocol allows finalized withdrawals to be executed permissionlessly, it would skip the wrapping of the ETH the cellar received, and the removal of the requestId. This would lead to an inaccurate requestId array, which could lead to considering withdrawn requests as pending balances, or cause issues with the balanceOf(). It would also lead to native tokens being deposited into the cellar which would be untracked.

Remediations to Consider

Resolution of this requires multiple adjustments:

  • Allow the ability to remove request ids from the requestIds array after verifying it has already been claimed, or in all of _completeBurns() implementations handle the already claimed case.
  • Ensure that a request hasn’t been claimed already in the implementations of _balanceOf() to prevent counting already claimed withdrawals
  • Create a nativeTokenAdaptor that tracks the cellar’s native token balance, and ensure that each cellar that uses a staking adaptor also has a position with this adaptor to make sure all withdrawals are still tracked.
L-1

SourceLocker can be deployed without a corresponding DestinationMinter

Topic
Validation
Status
Wont Do
Impact
Low
Likelihood
Low

To properly deploy SourceLocker and DestinationMinter, SourceLockerFactory needs to be connected with a corresponding DestinationMinterFactory. However, in the SourceLockerFactory.deploy method, there is no check to ensure that DestinationMinterFactory has been set. This can result in a successful deployment of SourceLocker without having a corresponding DestinationMinter, as the ccip message fails to reach the DestinationMinterFactory for deploying the DestinationMinter.

Remediations to Consider

Consider adding a check to SourceLockerFactory.deploy, to ensure the DestinationMinterFactory has been properly set.

Response by Sommelier

This mistake can only be made by the owner of the SourceLockerFactory, so we will fix this by properly setting up the factories before trying to set up a cross chain bridge.

L-2

Cellars do not support handling ERC1155 tokens

Topic
Use Cases
Status
Wont Do
Impact
Low
Likelihood
Low

Cellars currently inherit ERC721Holder, allowing them to receive ERC721 tokens. However, in its current state, the cellars don’t support receiving ERC1155 tokens. There could be protocols that issue ERC1155 instead of ERC721 tokens that strategists want to interact with.

Remediations to Consider

Add ERC1155Holder to the cellar’s inheritance chain

Response by Sommelier

There is not enough room in Cellar.sol to add the ERC1155Holder.sol. So if a future protocol does use ERC1155 we will re-evaluate adding in support for it.

Q-1

sourceChainSelector/destinationChainSelector not needed in SourceLocker/DestinationMinter

Topic
Unused Code
Status
Quality Impact
Low

In the SourceLocker contract, the sourceChainSelector variable is set in the constructor, but it is not used anywhere in the contract. Similarly, in the DestinationMinter contract, the destinationChainSelector variable is set in the constructor, but it is also not used anywhere in the contract.

Remediations to Consider

To improve readability, consider removing these unused state variables from the code.

Q-2

Improve events

Topic
Events
Status
Quality Impact
Low

In SourceLockerFactory’s deploy function, there is no event emitted. Consider emitting an event containing parameters such as target and locker address, messageId, etc.

Additionally, SourceLocker’s bridgeToDestination and DestinationMinter’s bridgeToSource function emit events that don’t include a messageId.

emit BridgeToDestination(amount, to);
emit BridgeToSource(amount, to);

It is recommended to include the messageId returned by router.ccipSend when emitting an event in the same function. This can be helpful when searching for sent messages in Chainlink’s CCIP Explorer.

Q-3

Allow SourceLocker and DestinationMinter contracts to be retrievable from factories

Topic
Composability
Status
Wont Do
Quality Impact
Medium

Currently the SourceLocker and DestinationMinter factories deploy the respective contracts and emit their addresses so they can be retrieved off-chain. However, there is no way for contracts on chain to easily know the contract address to interact with to bridge a particular cellar share token, without it being explicitly provided.

Remediations to Consider

Store the addresses of deployed contracts in the respective factories in a mapping with a key that can be used to easily retrieve either the SourceLocker or DestinationMinter contracts based on the token wanting to be bridged.

Response by Sommelier

This is a good convenience function that we can hash out for the more matured final version of this code.

Q-4

Bridging contracts should share the same interface

Topic
Composability
Status
Wont Do
Quality Impact
Low

The SourceLocker and DestinationMinter contracts both allow the bridging of tokens from one chain to the other, and share the same parameters.

/**
 * @notice Bridge shares back to source chain.
 * @dev Caller should approve LINK to be spent by this contract.
 * @param amount Number of shares to burn on destination network and unlock/transfer on source network.
 * @param to Specified address to burn destination network `share` tokens, and receive unlocked `share` tokens on source network.
 * @param maxLinkToPay Specified max amount of LINK fees to pay as per this contract.
 * @return messageId Resultant CCIP messageId.
 */
function bridgeToSource(uint256 amount, address to, uint256 maxLinkToPay) external returns (bytes32 messageId) {
    if (to == address(0)) revert DestinationMinter___InvalidTo();
    _burn(msg.sender, amount);

    Client.EVM2AnyMessage memory message = _buildMessage(amount, to);

    IRouterClient router = IRouterClient(this.getRouter());

    uint256 fees = router.getFee(sourceChainSelector, message);

    if (fees > maxLinkToPay) revert DestinationMinter___FeeTooHigh();

    LINK.safeTransferFrom(msg.sender, address(this), fees);

    LINK.safeApprove(address(router), fees);

    messageId = router.ccipSend(sourceChainSelector, message);
    emit BridgeToSource(amount, to);
}

Reference: DestinationMinter.sol#L87-L113

/**
 * @notice Bridge shares to destination chain.
 * @notice Reverts if target destination is not yet set.
 * @param amount number of `share` token to bridge.
 * @param to Specified address to receive newly minted bridged shares on destination network.
 * @param maxLinkToPay Specified max amount of LINK fees to pay.
 * @return messageId Resultant CCIP messageId.
 */
function bridgeToDestination(
    uint256 amount,
    address to,
    uint256 maxLinkToPay
) external returns (bytes32 messageId) {
    if (to == address(0)) revert SourceLocker___InvalidTo();
    shareToken.safeTransferFrom(msg.sender, address(this), amount);

    Client.EVM2AnyMessage memory message = _buildMessage(amount, to);

    IRouterClient router = IRouterClient(this.getRouter());

    uint256 fees = router.getFee(destinationChainSelector, message);

    if (fees > maxLinkToPay) revert SourceLocker___FeeTooHigh();

    LINK.safeTransferFrom(msg.sender, address(this), fees);

    LINK.safeApprove(address(router), fees);

    messageId = router.ccipSend(destinationChainSelector, message);
    emit BridgeToDestination(amount, to);
}

Reference: SourceLocker.sol#L113-L143

Although their execution differs slightly, one locks tokens and the other burns them, from the users perspective they function the same. Currently a user needs to know if their token originated on it’s chain or not in order to bridge it to another supported chain, since the interface of the contract differs. Having to determine where a share token originated before bridging may make interacting with these contracts confusing to integrate with.

Remediations to Consider

Standardize the bridging functions to share the same interface to make interacting with these contracts easier.

Response by Sommelier

This is a good convenience function that we can hash out for the more matured final version of this code.

Q-5

Allow for flexible input parameters in StakingAdaptor

Topic
Composability
Status
Quality Impact
Medium

StakingAdaptor is an abstract contract that is intended to be inherited by protocol specific staking adaptors, and it’s intent is to allow for these adaptors to all share the same interface, simplifying how strategist interacts with the varying staking protocol’s adaptors.

However, since StakingAdaptor is intended to be used by all any potential current or future protocol, there may be specific dynamic input parameters required. If that is the case, than another StakingAdaptor would need to be created with a differing interface.

Remediations to Consider

Add a bytes input parameter to the stakingAdaptors functions, and pass it into the internal functions that the specific protocol’s adaptors override. Doing this allow protocols to use this data and decode it as expected, giving the StakingAdaptor more flexibility to be interoperable with any staking protocol.

Q-6

Generic CompoundV3 adaptor names and identifiers

Topic
Consistency
Status
Quality Impact
Low

The CompoundV3 adaptors have generic names like BorrowAdaptor.sol and CollateralAdaptor.sol. These names are also used as their unique identifiers:

function identifier() public pure virtual override returns (bytes32) {
    return keccak256(abi.encode("Collateral Adaptor V 0.0"));
}

Reference: CollateralAdaptor.sol#L53-L55

This naming convention makes it difficult to determine what the adaptor is doing and what protocol it is interacting with.

Remediations to Consider

Update the names to be more specific, following the same adaptor naming convention already established.

Q-7

Inaccurate Comments

Topic
Documentation
Status
Quality Impact
Low
Q-8

Misleading parameter names in fulfilled event

Topic
Events
Status
Quality Impact
Low

In AtomicQueue.sol, the AtomicRequestFulfilled event is declared as follows:

event AtomicRequestFulfilled(
        address user,
        address offerToken,     
        address wantToken,
        uint256 sharesSpent,  
        uint256 assetsReceived,
        uint256 timestamp
    );

Reference: AtomicQueue.sol#L100-L101

In the context of the AtomicQueue, transferred tokens are defined as offer and want, rather than share and assets.

Remediations to Consider

For clarity sake, rename sharesSpent and assetsReceived parameters in the above event to something more meaningful, such as offerAmountSpent and wantAmountReceived.

G-1

Improving getAccountHealthFactor()

Topic
Gas optimization
Status
Gas Savings
Medium

V3Helper.sol’s getAccountHealthFactor() calculates the cellars health factor for a given market. However, it differs from how Compound calculates it health factor in isLiquidatable():

/**
 * @notice Check whether an account has enough collateral to not be liquidated
 * @param account The address to check
 * @return Whether the account is minimally collateralized enough to not be liquidated
 */
function isLiquidatable(address account) override public view returns (bool) {
    int104 principal = userBasic[account].principal;

    if (principal >= 0) {
        return false;
    }

    uint16 assetsIn = userBasic[account].assetsIn;
    int liquidity = signedMulPrice(
        presentValue(principal),
        getPrice(baseTokenPriceFeed),
        uint64(baseScale)
    );

    for (uint8 i = 0; i < numAssets; ) {
        if (isInAsset(assetsIn, i)) {
            if (liquidity >= 0) {
                return false;
            }

            AssetInfo memory asset = getAssetInfo(i);
            uint newAmount = mulPrice(
                userCollateral[account][asset.asset].balance,
                getPrice(asset.priceFeed),
                asset.scale
            );
            liquidity += signed256(mulFactor(
                newAmount,
                asset.liquidateCollateralFactor
            ));
        }
        unchecked { i++; }
    }

    return liquidity < 0;
}

Reference: Comet.sol#L541-581

Notably a collateral assets scale is used rather than querying the assets decimals(), and isInAsset() is checked to determine if the account has collateral of that asset type before doing any calculations.

Remediations to Consider

  • Pull the assetsIn for the cellar and use that to remove calculating the health factor for collateral the cellar doesn’t have deposited.
  • Use info.scale rather than retrieving the assets decimals to reduce external calls and ensure you are using the same value for the calculations as compound is.
  • Consider removing the maxNumberOfAssets check since only collateral deposited should be considered, reducing the gas costs for markets that accept many assets as collateral.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Sommelier team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.