Security Audit
March 20, 2024
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for Seven Seas's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from April 1st to April 5th 2024.
The purpose of this audit is to review the source code of certain Seven Seas 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 |
---|---|---|---|---|
Medium | 4 | - | - | 4 |
Low | 4 | 1 | - | 3 |
Code Quality | 4 | 1 | - | 3 |
Informational | 2 | - | - | - |
Seven Seas was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
Manager:
Intended to be the manageVaultWithMerkleVerification
contract, which allows verified callers to manage the BoringVault
with calls verified to be allowed via a set merkle tree.
Minter:
Intended to be set to be the TellerWithMultiAssetSupport
contract to mint new shares at a rate provided by the minter by calling enter()
on the BoringVault
.
Burner:
Intended to be set to be the TellerWithMultiAssetSupport
contract to burn shares in return for assets in the BoringVault
at a rate provided by the burner.
Internal Manager:
Will be set to be the manageVaultWithMerkleVerification
contract itself to call manageVaultWithMerkleVerification after receiving a balancer flashloan via the receiveFlashloan()
function.
Solver:
Will be a contract that implements the IAtomicSolver
interface, and controlled by an address with the strategist role. Can call bulkDeposit()
and bulkWithdrawal()
on the TellerWithMultiAssetSupport
contract.
Micromanager:
Role intended for micro manager contracts such as DexSwapperUManager
and DexAggregatorUManager
to call manageVaultWithMerkleVerification()
on the ManagerWithMerkleVerification
contract.
Owner:
This is an address that controls setting, updating, adding or removing important values for the BoringVault
, AccountantWithRateProviders
, ManagerWithMerkleVerification
, TellerWithMultiAssetSupport
, DexAggregatorUManager
, and DexSwapperUManager
contracts. It also controls the authority contract that determines the functions that roles can call throughout the protocol. This role is intended to be a timelocked multisig contract, with signers on the seven seas team as well as members of associated protocol.
Multisig:
A multisig contract that has some administrative functions with the ability to more quickly interact with the protocol than the timelocked owner does. Can pause and unpause the AccountantWithRateProviders
, TellerWithMultiAssetSupport
, and ManagerWithMerkleVerification
contracts as well as well as set allowed slippage, period and rate limit of the DexSwapperUManager
and DexAggregatorUManager
contracts. Controlled by signers on the seven seas team as well as members of associated protocol.
Strategist multisig:
Multisig that can call refundDeposit()
on the TellerWithMultiAssetSupport()
to return assets from a deposit that may have acted maliciously, like in cases of major arbitrage. Controlled by members of Seven Seas.
Strategist:
Main caller of manageVaultWithMerkleVerification()
on the ManagerWithMerkleVerification
contract, and may be able to call micro manager functions. Held by members of Seven Seas
Update exchange rate:
Role that allows setting the exchange rate in the AccountantWithRateProviders
via the updateExchangeRate()
function.
The following source code was reviewed during the audit:
939c77e25473dff3ed18fa104f004f7afd13452e
Specifically, we audited the following contracts within this repository.
Contract | SHA256 |
---|---|
src/atomic-queue/AtomicQueue.sol |
|
src/atomic-queue/AtomicSolver.sol |
|
src/atomic-queue/IAtomicSolver.sol |
|
src/base/BoringVault.sol |
|
src/base/DecodersAndSanitizers/BaseDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/EtherFiLiquidDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/LidoLiquidDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/AaveV3DecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/AuraDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/BalancerV2DecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/ConvexDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/CurveDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/ERC4626DecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/EtherFiDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/GearboxDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/LidoDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/MorphoBlueDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/NativeWrapperDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/OneInchDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/PendleRouterDecoderAndSanitizer.sol |
|
src/base/DecodersAndSanitizers/Protocols/UniswapV3DecoderAndSanitizer.sol |
|
src/base/Roles/AccountantWithRateProviders.sol |
|
src/base/Roles/ManagerWithMerkleVerification.sol |
|
src/base/Roles/TellerWithMultiAssetSupport.sol |
|
src/helper/BoringVaultV0Lens.sol |
|
src/interfaces/AggregationRouterV5.sol |
|
src/interfaces/BalancerVault.sol |
|
src/interfaces/BeforeTransferHook.sol |
|
src/interfaces/DecoderCustomTypes.sol |
|
src/interfaces/EtherFiLiquid1.sol |
|
src/interfaces/IRateProvider.sol |
|
src/interfaces/IStaking.sol |
|
src/interfaces/IUniswapV3Router.sol |
|
src/interfaces/PriceRouter.sol |
|
src/interfaces/RawDataDecoderAndSanitizerInterfaces.sol |
|
src/micro-managers/DexAggregatorUManager.sol |
|
src/micro-managers/DexSwapperUManager.sol |
|
src/migration/CellarMigrationAdaptor.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.
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. |
The ManagerWithMerkleVerification
contract allows permissioned addresses, typically strategists, to manage the BoringVault
after verifying the calls are valid to make. However, in the case where a strategist starts to act against the best interest of the vault, there are no measures in place to quickly mitigate the damage. Currently the owner role can remove the merkle root associated with a strategist, but this action is behind a timelock and cannot act quickly if required.
Remediations to Consider
Allow pausing and unpausing strategists from managing the vault via a non-timelocked trusted role to allow quick action if a strategist goes rogue. Also consider rate limiting strategist actions, to reduce the potential impact of a rogue strategist.
In UniswapV3DecoderAndSanitizer.sol
the functions increaseLiquidity
and decreaseLiquidty
, and collect()
, the tokens in the pool associated with the provided tokenId are not checked to see if they are supported, unlike for the functions exactInput()
and mint()
. Although ownership of the provided token id is checked to be the vault, adding liquidity to this pool may be unauthorized seeing as anyone can create a lp position and transfer it to the vault. This allows strategists to take up unexpected lp positions, and or collect unexpected rewards.
Remediations to Consider
Check with the uniswap position manager which tokens are related to the token id, and add these tokens to the returned addressesFound array to be verified via the merkle tree.
In TellerWithMultiAssetSupport.sol
the deposit()
and depositWithPermit()
functions require the provided depositAsset
is supported, by checking it with the isSupported
mapping.
However, the functions bulkDeposit()
and bulkWithdraw()
do not make this check. Although both of these functions are permissioned, there may be multiple entities allowed to call these functions, including some select other vaults in the case of the CellarMigrationAdaptor
, where ensuring the requested assets are allowed to be deposited and or withdrawn.
Remediations to Consider
Check the isSupported
mapping if the depositAsset
or withdrawAsset
is supported before allowing bulk deposit or withdrawals.
The AccountantWithRateProvider.sol
contract, allows setting an exchange rate of a Boring Vaults shares based in a base asset, using the base assets decimals. This contract also allows getting the exchange rate denominated in a provided quote asset, if it is supported, by calling getRateInQuote(), or a strategist can claimFees() in a provided asset. In both these cases the exchange rate needs to be converted from a rate of base/share to quote asset/share. However, some assets are pegged to the base asset, meaning their price is tied and assumed to be exchanged 1:1, but currently it is assumed that their decimals are also the same:
...
if (address(feeAsset) == address(base) || data.isPeggedToBase) {
feesOwedInFeeAsset = state.feesOwedInBase;
}
...
Reference: AccountantWithRateProvider.sol#L284-286
if (data.isPeggedToBase) {
rateInQuote = accountantState.exchangeRate;
}
Reference: AccountantWithRateProvider.sol#L325-327
If the decimals are different for the quote asset than the base it can lead to an inaccurate value.
This is also the case for non-pegged assets, where the exchange rate is determined by calling getRate()
on an associated rate provider, but is is assumed that the quote asset has the same decimals as the base asset as well, leading to the same issue.
Using inaccurate decimals for these functions results in inaccurate fees claimed or the rate returned, leading to either more or less fees collected, or more or less shares received when depositing. This can be remedied with refunding deposits if caught, and assuming a strategist is trusted could have inaccurate fees received covered or returned.
Remediations to Consider
Convert these values from the base assets decimals to the quote assets decimals for both pegged and non-pegged assets when claiming fees or getting the exchange rate in a quote asset.
When a transaction gets submitted to an RPC it is typically added to the mempool and can stay there for a indefinite amount of time, and can be delayed for a number of reasons. Sometimes malicious MEV/block builders may hold off executing a transaction until it suits them. Other times the gas price submitted may not be sufficient to compete to other demand, and execution may be held until competitive gas prices drop back down, but for time or condition sensitive transactions, this delay may result in the user no longer wanting to execute the transaction.
Protocols like UniswapV3 take an execution deadline as a parameter to give users control over this, however in the DexSwapperUManager
micro manager, the deadline is set to the block.timestamp
, which means it will always be a valid time, negating it set purpose:**
IUniswapV3Router.ExactInputParams memory params = IUniswapV3Router.ExactInputParams({
path: packedPath,
recipient: boringVault,
deadline: block.timestamp,
amountIn: amountIn,
amountOutMinimum: amountOutMinimum
});
Reference: DexSwapperUManager.sol#L115-L121
Remediations to Consider
Add an additional deadline parameter to ManagerWithMerkleVerification.sol
’s manageVaultWithMerkleVerification()
function, and require that the deadline hasn’t been exceeded, to give control over the timing of vault managements. Alternatively, for protocol interactions that are known to be timing sensitive, require a deadline in its corresponding micro manager.
In ManagerWithMerkleVerification.sol
, the function flashLoan()
is intended to be called by the BoringVault
that it manages, after being initiated by manageVaultWithMerkleVerification()
, which initiates a balancer flashloan where the balancer vault calls receiveFlashloan()
. Both of these functions have a explicit intended caller that should not change, and these addresses are currently immutable values within the ManagerWithMerkleVerification
contract. However, authorization is determined using the onlyAuth
modifier which allows for the authorized addresses to change.
Remediations to Consider
Instead of using the onlyAuth
modifier for flashLoan()
and receiveFlashloan()
, explicitly require the caller to be the vault
and balancerVault
addresses respectively.
In AccountantWithRateProviders.sol
, a permissioned entity is allowed to call updateExchangeRate()
to update the vaults exchange rate, based on the value of assets in the vault per share. They are limited to only updating the exchange rate every so often and is defined by the minimumUpdateDelayInHours
, otherwise attempting to update it before will can cause the vault to be paused:
if (
currentTime < state.lastUpdateTimestamp + (state.minimumUpdateDelayInHours * ONE_HOUR) || newExchangeRate > currentExchangeRate.mulDivDown(state.allowedExchangeRateChangeUpper, 1e4)
|| newExchangeRate < currentExchangeRate.mulDivDown(state.allowedExchangeRateChangeLower, 1e4)
) {
// Instead of reverting, pause the contract. This way the exchange rate updater is able to update the exchange rate
// to a better value, and pause it.
state.isPaused = true;
}
Reference: AccountantWithRateProviders.sol#L237-L244
This delay is limited to being defined in hours, which may not be as precise as a vault requires. Currently the minimumUpdateDelayInHours
is packed in a storage slot as a unit8, but there is enough room in the slot to be up to a uint40 which could allow this delay to be reasonably expressed in seconds.
Remediations to Consider
Adjust the minimum delay to be defined in seconds to give more flexibility to the exchange rate updater.
BoringVaults are setup to potentially borrow assets from protocols like Morpho blue and Aave by posting collateral. Borrowing assets is risky, since the collateral put up has to be worth more than the value of the borrowed amount, otherwise the collateral will be liquidated. Most protocols ensure the value of collateral to loan, the health factor, is above the liquidation threshold, however setting an unhealthy health factor adds a lot of risk to the vault of losing assets from having the position liquidated, which may want to be prevented. The assumption is that strategists will act to the vaults benefit, but without additional checks in place a strategist may create a more risky loan than intended.
Remediations to Consider
Add micro managers that ensure positions maintain a sufficiently high health factor for all loans made to protocols.
This issue will be addressed in future audits, by using a uManager.
In the prior vault managing protocol, cellar functions allowed strategists to specify they wanted to use all of a particular asset in the vault to do some action, which made interacting with the vault easier than providing exact amounts, which could vary throughout a set of rebalance actions. However, in this version that functionality is missing which is a quality of life decrease from the prior version.
Remediations to Consider
Add the ability for a strategist to specify the max amount of an asset the valut has available to use for relevant actions. Consider handling this in the sanitizers and decoders as an extra argument, and returning this additional data.
This issue will be addressed in future audits.
Events like deposit
in the TellerWithMultiAssetSupport
contract emits relevant deposit info, like the recipient
, the depositAsset
which could be used by front-end applications or services to more easily query and display a users actions and assets if these values were indexed.
Remediations to Consider
Index relevant event parameters in events like the teller’s deposit
and the BoringVault
’s enter
and exit
events.
Authorized functions throughout the boring vault repo use solmate’s Auth’s requiresAuth modifier to check if the sender is authorized to call the specific function. However, there are multiple potential roles that could be authorized but it is not explicitly stated which role is expected to call each function
Remediations to Consider
Add the intended role to the Natspec of each authorized function to make the intention more clear.
In ManagerWithMerkleVerification.sol’
s manageVaultWithMerkleVerification()
strategists pass in data for function calls they would like the corresponding vault to make, along with a proof that each call is in a merkle tree and thus allowed. When each of these function calls is verified it calls _verifyCalldata()
which will revert with ManagerWithMerkleVerification__FailedToVerifyManageProof
if a call is not successfully verified.
However, this error does not return any information about the invalid function call and callsmanageVaultWithMerkleVerification()
may take multiple function calls, which may make it difficult for a strategist to determine which of their calls is invalid.
Remediations to Consider
Add additional parameters to the ManagerWithMerkleVerification__FailedToVerifyManageProof
that make it clear which call has failed, making it easier for strategists to debug complicated manage calls.
Although every call a BoringVault
makes is verified by the set merkle tree for a given caller, these verification checks are not a catch all to prevent malicious behaviour from a strategist. Users that invest in the vault still have to trust that the managers will act in the best interest of the vault and it’s shareholders.
This issue will be addressed by introducing a more decentralized setup to sanitize rebalance TXs.
Since the vault does not check the value held in the vault before and after a call to manage, there could be protocol interactions that could be exploited via MEV, such as sandwich attacks or front-running, that may negatively impact the vault. It is important that this is considered and all calls to manage the vault be done so through a trusted private RPC to reduces the likelihood of being negatively effected by MEV.
Rebalance TX will always be submitted using a trusted block builder.
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 Seven Seas 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.