ABSTRACT
Dedaub was commissioned to perform a security audit of the Rysk Ciao protocol.
Ciao operates as a spot and perpetual trading protocol. To begin, users deposit funds into the Ciao.sol contract using assets permitted by the owner. From there, users can initiate spot or perpetual positions. The protocol employs an off-chain order book and matching algorithm to facilitate order matching. Users provide signed order details to the off-chain system, which then either fulfills the order fully or partially, or places it in the order book. Before order matching, the off-chain system calculates the user’s overall health by considering all open positions and collateral. If a user’s health drops below zero, anyone can liquidate their position at a discounted price. On-chain verification ensures that the liquidated health is indeed negative before the liquidation occurs, and afterward, it confirms that the health is not strictly positive, indicating there was no over-liquidation.
During this audit, we exclusively examined the on-chain aspects of the protocol. We did not have access to the off-chain system responsible for implementing the order book and the order-matching algorithm. All the information we obtained about this part of the system was provided to us through discussions with the team.
SETTING & CAVEATS
This audit report mainly covers the contracts of the at-the-time private repository rysk-ciao-protocol at commit 29a054b4d333de8ca35cb037f259250f1144434b. Audited suggested fixes were also reviewed up to commit hash c61538830e2e60d9ef47d305b48ab0a5cbdb939d. Three auditors worked on the codebase for 10 days.
The test suite was consulted during the audit but was not part of it. The full list of audited files is:
contracts//
├── AddressManifest.sol
├── Ciao.sol
├── crucible/
│ ├── Crucible.sol
│ ├── move-crucible/
│ │ └── MoveCrucible.sol
│ ├── perp-crucible/
│ │ └── PerpCrucible.sol
│ └── spot-crucible/
│ └── SpotCrucible.sol
├── Furnace.sol
├── interfaces/
│ ├── Errors.sol
│ ├── Events.sol
│ ├── IAddressManifest.sol
│ ├── ICiao.sol
│ ├── ICrucible.sol
│ ├── IFurnace.sol
│ ├── ILiquidation.sol
│ ├── IOrderDispatch.sol
│ ├── IPerpCrucible.sol
│ └── IProductCatalogue.sol
├── libraries/
│ ├── AccessControl.sol
│ ├── BasicMathInt.sol
│ ├── BasicMathUint.sol
│ ├── Commons.sol
│ ├── EnumerableSet.sol
│ ├── MarginDirective.sol
│ └── Parser.sol
├── Liquidation.sol
├── OrderDispatch.sol
├── ProductCatalogue.sol
└── readers/
├── MarginReader.sol
└── UserAndSystemStateReader.sol
The audit’s main target is security threats, i.e., what the community understanding would likely call “hacking”, rather than the regular use of the protocol. Functional correctness (i.e. issues in “regular use”) is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.
PROTOCOL-LEVEL CONSIDERATIONS
The protocol heavily depends on the correct operation of its off-chain component
Protocol Level Consideration | Status: INFO
The off-chain system is pivotal to the protocol’s operation. It maintains the order book and executes the matching algorithm, vital components that we haven’t reviewed in this audit. Consequently, we cannot ascertain several aspects, such as how the off-chain system chooses among multiple matching options for a taker, which can significantly impact the system’s optimality and performance.
Furthermore, the off-chain system is entrusted with providing accurate and up-to-date prices, along with other crucial parameters, essential for the protocol’s integrity and functionality.
A third and equally crucial task of the off-chain system is to compute the impact of each action on the health of all the involved sub-accounts before executing the action, ensuring that an action will be executed only if it does not decrease the health of an account below 0. The protocol team has implemented certain reader contracts that implement the sub-account health calculations and could be called off-chain to aid the off-chain system and offer more transparency.
It’s evident that if the off-chain system fails to perform any of these roles effectively, the consequences for the system would be severe. At the same time, users of the protocol have to trust the off-chain system to execute their actions in a timely and fair manner, as the vast majority of user actions cannot be performed by directly calling the protocol contracts but have to go through the off-chain system instead.
VULNERABILITIES & FUNCTIONAL ISSUES
This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:
Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.
CRITICAL SEVERITY
User signatures for certain actions can be reused
Critical | Status: RESOLVED
The system expects users to sign messages to perform certain actions, as the off-chain component is the only entity that can perform these actions on-chain (call the contracts). These actions include deposits, withdrawals, signer approvals, orders, liquidations. However, the system does not directly invalidate the provided signature, once the corresponding user action has been performed. This could allow any malicious user who observes a submitted signature to submit it again, i.e., replay past signatures, negatively affecting the original signer in various ways.
As an example, a withdrawal could be replayed by a malicious actor, i.e., the original signer of the withdrawal message could be forced to withdraw their collateral from the protocol at any point, even multiple times. Such an action would decrease the user’s health without their consent, potentially leading to a liquidation, a kind of griefing attack. Similarly, deposit signatures could be replayed if a depositor has given a big enough allowance to the Ciao contract. A signer approval could also be replayed to re-approve a no-longer-approved signer. Liquidations exhibit the same issue in contrast to orders, which are not affected as their fulfillment is tracked on chain, disallowing potential replays.
In addition, the contracts do not offer a way for users to directly invalidate past signatures, either used or unused ones.
HIGH SEVERITY
Changing the core collateral breaks health calculations
High | Status: RESOLVED
The Ciao method setCoreCollateralAddress() allows the owner to update the core collateral after the protocol’s deployment. Such a change would create problems with the protocol’s sub-account health calculations.
One way the health calculation would be affected can be observed in the following snippet:
Furnace::getSubAccountHealth():158
tempVars.spotAssets = _ciao().getSubAccountAssets(subAccount);
tempVars.assetsLen = tempVars.spotAssets.length;
// Dedaub: Code omitted for brevity
for (uint i = 0; i < tempVars.assetsLen; i++) {
address spotAssetAddress = tempVars.spotAssets[i];
uint32 spotProductId = _productCatalogue()
.baseAssetQuoteAssetSpotIds(
spotAssetAddress,
_ciao().coreCollateralAddress()
);
uint256 spotBalance = _ciao().balances(
subAccount,
spotAssetAddress
);
if (spotProductId == CORE_COLLATERAL_INDEX) { // ←------
health +=
int256(spotBalance) -
int256(_ciao().coreCollateralDebt(subAccount));
continue;
}
}
The code assumes that the spotProductId for the pair (coreCollateral, coreCollateral) will be equal to CORE_COLLATERAL_INDEX which is a constant set to 1. However, as the ProductCatalog contract does not offer a way to update the base asset and quote asset for a given productId, the productId of 1 will forever be that of the initial core collateral asset.
Even if the core collateral spotProductId was configurable there would be no way to ensure that a user’s subAccountAssets EnumerableSet would include the new core collateral asset. In that case, users would have to perform some action (or have some action performed to their sub-account which would require either Ciao::settleCoreCollateral() to be called or Ciao::_updateBalance() with the new coreCollateral as a param) in order to have it added. If getSubAccountHealth() was to be called prior to that, the call to _ciao().getSubAccountAssets(subAccount) would not include the new coreCollateral, thus the coreCollateralDebt would not be included in the health calculation.
Users can withdraw their deposits without any restrictions or health checks after a specified waiting period
High | Status: RESOLVED
There are two methods for withdrawal: (1) users can submit a signed withdrawal request to the off-chain system, where it will be verified (off-chain only) to ensure it doesn’t put at risk the user’s subaccount health (and subsequently to the protocol), or (2) they can directly call Ciao::requestWithdrawal to specify the asset and amount they wish to withdraw and create a withdrawal receipt. After a designated minimumWithdrawalWaitTime from the receipts creation, a user can execute the withdrawal using Ciao::executeWithdrawal without any restriction and without undergoing any health checks. This poses a risk to the protocol, as a withdrawal of a large amount of collateral could result in bad debt, since the subaccount’s health could be significantly negative after the execution of the withdrawal.
Addressing this issue presents a challenge as there is a trade-off between user’s freedom and protocol health. One approach could be to restrict manual withdrawals and require users to go through the off-chain system to withdraw, in which case all the necessary health checks should be performed. However, this may be overly restrictive, as there’s no guarantee the off-chain system will promptly process requests. Alternatively, additional checks could be implemented in the manual withdrawal process, such as ensuring that the subaccount’s health remains above a certain threshold after the withdrawal. Finding the right balance between user autonomy and protocol stability is key in resolving this issue.
MEDIUM SEVERITY
A user can open an extensive number of small positions, making the liquidation of their main position economically non-viable
Medium | Status: ACKNOWLEDGED
Each user sub-account can hold multiple positions, one for each available asset. The overall health of a sub-account considers all these positions. If the health turns negative, certain restrictions are theoretically in place, such as the prevention of opening new positions. However, these restrictions are not enforced on-chain. Instead, the protocol relies on the off-chain system to monitor subaccount health and refrain from executing actions when it is negative.
The on-chain computation and utilization of a sub-account’s health occurs solely during liquidations. In the final step of a liquidation, it is confirmed that the (initial) health of the liquidated sub-account after the process is not positive, ensuring the absence of over-liquidation, and the liquidator’s sub-account health has remained positive after the liquidation. However, this on-chain process involves significant gas costs, as the contract iterates over all the open positions of the sub-account to calculate its health, particularly during the comparison of spot and perpetual positions to determine the spread.
A user can exploit this system by taking advantage of the absence of a minimum required amount to open a position. They can open numerous tiny positions, one for each approved asset. In scenarios where the list of approved assets is extensive, the gas cost associated with liquidating such a sub-account becomes substantial and may render liquidation economically infeasible for the potential liquidator. Moreover, the excessive gas required could lead to an out of gas exception and hinder the possibility of executing a liquidation.
As a recommended mitigation, we propose the implementation of a minimum position size, preventing users from opening tiny positions and mitigating the risk of potential abuse.
The current withdrawal process only checks for the presence of a withdrawal receipt but does not verify the actual amount specified in the receipt
Medium | Status: RESOLVED
Users can request a withdrawal from their account either directly through the contract or via the off-chain system. In the latter case, the user must provide a valid signature or have previously created a withdrawal receipt in the contract, documenting the asset and the withdrawal amount. However, when there is no valid signature, the OrderDispatch::withdraw() function only confirms the existence of an active withdrawal receipt without validating that the amount matches the requested withdrawal amount.
We recommend incorporating a verification step to ensure that the amount specified in the withdrawal receipt matches the requested withdrawal amount. Alternatively, an enhancement could involve passing the withdrawal receipt amount instead of the requested amount in the Ciao.executeWithdrawal function.
LOW SEVERITY
The current system doesn’t include the order’s expiration when users sign messages
Low | Status: RESOLVED
In most cases, users interact with the protocol through the off-chain system. They submit details of their intended actions, sign the hash of the resulting message following EIP-712, and the off-chain system sends these details to the OrderDispatch contract for verification, including the user’s signature. Specifically for orders, users submit and sign data structured as follows:
Commons.sol:50
struct Order {
address account;
uint8 subAccountId; // id of the sub account being used
uint32 productId;
bool side; // 0 for sell, 1 for buy
Designation designation;
uint64 expiration;
uint128 price;
uint128 amount;
uint64 nonce;
}
However, in the OrderDispatch::getOrderDigest() function, the expiration variable is missing in the hashed message. This absence raises concerns about whether the user-submitted expiration matches the on-chain one provided by the off-chain system.
Despite the current non-use of expiration for on-chain checks (likely checked off-chain only), it is recommended to include the expiration field in the hashed message.
The numOfPosOpen of each sub-account is always zero
Low | Status: RESOLVED
The mapping Crucible.numOfPosOpen, which associates each sub-account with its count of open positions, is never updated. Consequently, it consistently returns zero for every subaccount. While this mapping is not presently utilized for accounting or checks within the contracts, it is accessible to users through the viewer to verify their number of positions. As a best practice, it should accurately reflect the correct value.
To address this, we recommend updating it in the PerpCrucuble::_updatePosition function accordingly.
Inconsistencies in the number of decimals for asset amounts between different protocol actions
Low | Status: ACKNOWLEDGED
Functions deposit, requestWithdrawal and executeWithdrawal expect the amount of the asset to be denominated in the number of decimals of the asset and then use Commons::convertToE18 to convert the amount to 18 decimals. On the other hand, all the other actions (MatchOrder, UpdateProductPrice, ForceSwap, Liquidate and UpdateCumulativeFunding) that process asset amounts assume that the amount will be denominated in 18 decimals. All these actions are controlled solely from the off-chain component except for Liquidate, thus liquidateSubAccount could be directly called by a user, who might assume that the same decimals convention used for deposits and withdrawals should be used. We believe that these inconsistencies should be eliminated or clearly documented to avoid trivial user (or even system) errors.
The validity of the product id is not checked when updating asset prices or the funding rate
Low | Status: ACKNOWLEDGED
Functions Furnace::setPrices and PerpCrucible::updateCumulativeFundings do not check that the provided productId corresponds to an existing registered product in the ProductCatalogue contract.
CENTRALIZATION ISSUES
It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)
The off-chain system provides the prices for the assets and controls other critical system parameters
Centralization | Status: ACKNOWLEDGED
The mark prices are not obtained from an on-chain oracle but are instead sourced from the off-chain system. As a result, there is no guarantee of their timeliness or accuracy. While the team assured us that the off-chain system will retrieve prices from Stork oracles and update them in a timely manner, there are currently no on-chain checks that verify the accuracy of the pushed prices.
Similarly, the off-chain system is tasked with setting and updating other vital parameters such as the funding rate, fees, and weights used in health computations, without on-chain verification of their values or update times. During this audit, we operated under the assumption that the provided prices and parameters would be reasonable. However, it’s worth noting that a malicious owner could potentially exploit this lack of on-chain validation through various means (e.g., by artificially creating unfavorable economic conditions) to seize control of the protocol and essentially users’ funds.
Except for liquidations, account health check are performed only by the off-chain component
Centralization | Status: ACKNOWLEDGED
The protocol defines two types of health, maintenance and initial, for each sub-account. When a sub-account’s initial health drops below 0, they are not allowed to open any positions which reduce their health further, while when a sub-account’s maintenance health drops below 0, they become liquidatable. Both the initial and maintenance healths can be calculated using the same getSubAccountHealth() of the Furnace contract. However, since these calculations are expensive in terms of gas, the protocol team has decided that these criteria are to be asserted by the off-chain system for all actions (MatchOrder, ForceSwap and ExecuteWithdrawal) except for the Liquidate action, as this is the only action that can be executed without going through the off-chain component. To be precise, withdrawals can be also performed without going through the off-chain component, i.e., without checking a sub-account’s health, but this is further discussed in issue H2. All in all, It is assumed that the off-chain component will always simulate off-chain the getSubAccountHealth() calculation and that it will only perform actions that are permitted based on that calculation.
OTHER / ADVISORY ISSUES
This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.
Getter methods are defined twice
Advisory | Status: INFO
A number of getter methods are defined both in the Commons library and the AccessControl abstract contract. Namely methods:
Commons::ciao()Commons::furnace()Commons::productCatalogue()Commons::orderDispatch()Commons::liquidation()Commons::perpCrucible()
are also defined respectively as:
AccessControl::_ciao()AccessControl::_furnace()AccessControl::_productCatalogue()AccessControl::_orderDispatch()AccessControl::_liquidation()AccessControl::_perCrucible()
Getters in Commons.sol can be set to internal
Advisory | Status: INFO
The spotCrucible(), perpCrucible(), moveCrucible(), and crucible() getter methods in the Commons library have their visibility set to external. As the body of these methods is essentially only an external call, changing their visibility to internal will provide gas savings.
Ciao::_settleCoreCollateral could return early in certain cases
Advisory | Status: INFO
The function settleCoreCollateral() of the Ciao contract could return early if coreCollateralAmount == 0.
View method can successfully return for invalid input
Advisory | Status: INFO
View function ProductCatalogue::isProductType() can succeed for productType == 0 and an invalid productId.
ProductCatalogue::isProductType():132
function isProductType(
uint32 productId,
uint8 productType
) external view returns (bool) {
return products[productId].productType == productType;
}
Although currently supported products have a productType of 1, 2, and 3, the code of the ProductCatalogue::setProduct() method allows a productType of 0 to be used.
Product.isProductTradeable field is never checked
Advisory | Status: INFO
The isProductTradeable field of the Product struct is stored on the products mapping of the ProductCatalogue contract and the function ProductCatalogue::changeProductTradeability() exists to allow the admin to update the isProductTradeable field value for a given productId. However, this field is never checked on-chain.
Inconsistency between interface and implementation contract
Advisory | Status: INFO
The first two arguments of IPerpCrucible::updatePosition are named makerSubAccount and takerSubAccount, respectively. However, in PerpCrucible::updatePosition, the order of these arguments is reversed. This inconsistency also applies to the order of the output arguments.
Ciao::_withdraw does not explicitly check the contracts balance
Advisory | Status: INFO
The function Ciao::_withdraw could check that the amount to be withdrawn is less than or equal to the balance of the Ciao contract before attempting the safeTransfer call in order to provide a user-friendly revert message.
Missing sanity checks in MarginReader::getSubAccountMargin
Advisory | Status: INFO
In MarginReader::getSubAccountMargin it’s advisable to include a check within the for loop that computes spreads, ensuring that the spread penalty has been set and is non-zero. Similarly, within the for loop that computes the health of the perpetual positions, it’s recommended to add a check to ensure that the relevant weights are non-zero.
Signature type hashes are recomputed on every transaction
Advisory | Status: INFO
The type hashes for the signatures of all the supported actions are computed on the spot, when the digests are produced.
An example can be seen below:
string constant DEPOSIT =
"Deposit(address account,uint8 subAccountId,address asset,uint256 amount,uint64 nonce)";
function getDepositDigest(
Commons.Deposit memory depo
) public view returns (bytes32) {
return
_hashTypedDataV4(
keccak256(
abi.encode(
keccak256(bytes(DEPOSIT)),
depo.account,
depo.subAccountId,
depo.asset,
depo.amount,
depo.nonce
)
)
);
}
The type hashes can be directly defined as constants instead, saving both gas and bytecode size.
Compiler bugs
Advisory | Status: INFO
The code is compiled with Solidity 0.8.22. Version 0.8.22, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.
DISCLAIMER
The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.
ABOUT DEDAUB
Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub’s auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.