Security Audit
February 12, 2024
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
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.
The following source code was reviewed during the audit:
f7a1613012177619f76c1cf9da203feacd53856b
6020dae9399062bc8c1ee86fd9a78573ee7290de
Specifically, we audited the following contracts within this repository.
Contract | SHA256 |
---|---|
src/modules/multi-chain-share/DestinationMinter.sol |
|
src/modules/multi-chain-share/DestinationMinterFactory.sol |
|
src/modules/multi-chain-share/SourceLocker.sol |
|
src/modules/multi-chain-share/SourceLockerFactory.sol |
|
src/modules/adaptors/Compound/V3/BorrowAdaptor.sol |
|
src/modules/adaptors/Compound/V3/CollateralAdaptor.sol |
|
src/modules/adaptors/Compound/V3/RewardsAdaptor.sol |
|
src/modules/adaptors/Compound/V3/SupplyAdaptor.sol |
|
src/modules/adaptors/Compound/V3/V3Helper.sol |
|
src/modules/adaptors/Staking/EtherFiStakingAdaptor.sol |
|
src/modules/adaptors/Staking/KelpDAOStakingAdaptor.sol |
|
src/modules/adaptors/Staking/LidoStakingAdaptor.sol |
|
src/modules/adaptors/Staking/RenzoStakingAdaptor.sol |
|
src/modules/adaptors/Staking/StaderStakingAdaptor.sol |
|
src/modules/adaptors/Staking/StakingAdaptor.sol |
|
src/modules/adaptors/Staking/SwellStakingAdaptor.sol |
|
src/modules/price-router/Extensions/EtherFi/eETHExtension.sol |
|
src/modules/atomic-queue/AtomicQueue.sol |
|
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.
Click on an issue to jump to it, or scroll down to see them all.
SourceLocker
can be deployed without a corresponding DestinationMinter
sourceChainSelector
/destinationChainSelector
not needed in SourceLocker
/DestinationMinter
SourceLocker
and DestinationMinter
contracts to be retrievable from factories
StakingAdaptor
getAccountHealthFactor()
We quantify issues in three parts:
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. |
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,
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.
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.
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.
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
)
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.
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 thatextraArgs
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.
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
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:
requestIds
array after verifying it has already been claimed, or in all of _completeBurns()
implementations handle the already claimed case._balanceOf()
to prevent counting already claimed withdrawalsSourceLocker
can be deployed without a corresponding DestinationMinter
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.
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.
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
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.
sourceChainSelector
/destinationChainSelector
not needed in SourceLocker
/DestinationMinter
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.
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.
SourceLocker
and DestinationMinter
contracts to be retrievable from factories
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.
This is a good convenience function that we can hash out for the more matured final version of this code.
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.
This is a good convenience function that we can hash out for the more matured final version of this code.
StakingAdaptor
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.
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.
assetsToOffer
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
.
getAccountHealthFactor()
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
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.maxNumberOfAssets
check since only collateral deposited should be considered, reducing the gas costs for markets that accept many assets as collateral.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.