Low Risk Report for Zaros by K42
Low Severity Issues
| Issue ID |
Description |
Location Link |
Impact |
Recommended Mitigation Steps |
| L-01 |
No use of two-step ownership transfer in BaseKeeper |
BaseKeeper.sol |
Low |
Use a two-step ownership transfer process |
| L-02 |
Unbounded iterations in checkLiquidatableAccounts |
LiquidationKeeper.sol |
Low |
Maximum iteration limit |
| L-03 |
No pause mechanism in MarketOrderKeeper |
MarketOrderKeeper.sol |
Low |
For safety and prevention, use pausable pattern for critical functions here |
| L-04 |
Centralization risk in configureSystemParameters |
GlobalConfigurationBranch.sol |
Low |
Add in a time-lock or multi-sig mechanism for critical parameter changes |
| L-05 |
Risk of DOS in liquidateAccounts due to gas limit |
LiquidationBranch.sol |
Low |
Batched liquidations |
| L-06 |
Lack of minimum order size check beyond zero in createMarketOrder |
OrderBranch.sol |
Low |
Add in a minimum order size check |
| L-07 |
Risk of precision loss in getMarkPrice calculation |
PerpMarketBranch.sol |
Low |
Use higher precision intermediates in current calculations |
| L-08 |
No signature malleability protection in fillOffchainOrders |
SettlementBranch.sol |
Low |
EIP-2098 for signature compaction and malleability protection |
| L-09 |
Risk of front-running in createTradingAccount |
TradingAccountBranch.sol |
Low |
Better use of commit-reveal scheme for account creation |
| L-10 |
Not enough event emission for critical configuration changes here |
GlobalConfiguration.sol |
Low |
Emit events for more of the critical configuration changes |
| L-11 |
Vector for price manipulation in getPrice |
MarginCollateralConfiguration.sol |
Low |
TWAP or median price mechanism |
| L-12 |
No slippage protection in getMarkPrice |
PerpMarket.sol |
Low |
Slippage protection parameters |
| L-13 |
Risk of unexpected results with extremely large position sizes or prices in getNotionalValue |
Position.sol |
Low |
More precise checks for extremely large values |
| L-14 |
Centralization risk in verifyDataStreamsReport |
SettlementConfiguration.sol |
Low |
Multiple data sources or fallback mechanism |
| L-15 |
No gas limit for external calls in deductAccountMargin |
TradingAccount.sol |
Low |
Gas limits for external calls |
| L-16 |
Vector of reentrancy in initializeRootUpgrade |
RootUpgrade.sol |
Low |
Reentrancy guard for initialization process to be extra safe |
Detailed Breakdown of All Low Severity Issues:
L-01: No use of two-step ownership transfer in BaseKeeper
Proof of Concept
In BaseKeeper.sol, the ownership transfer is done in a single step, which can be risky if the new owner address is incorrect:
function __BaseKeeper_init(address owner) internal onlyInitializing {
if (owner == address(0)) {
revert Errors.ZeroInput("owner");
}
__Ownable_init(owner);
}
Impact
If the owner address is set incorrectly, by accident the contract could become permanently ownerless, locking critical functionalities.
Recommended Mitigation Steps
Use the method of a two-step ownership transfer process:
address private _pendingOwner;
function transferOwnership(address newOwner) public virtual onlyOwner {
_pendingOwner = newOwner;
emit OwnershipTransferInitiated(owner(), newOwner);
}
function acceptOwnership() public virtual {
require(msg.sender == _pendingOwner, "Only pending owner can accept ownership");
_transferOwnership(_pendingOwner);
_pendingOwner = address(0);
}
L-02: Unbounded loop in checkLiquidatableAccounts
Proof of Concept
In LiquidationKeeper.sol, the checkLiquidatableAccounts function contains unbounded loop:
for (uint256 i = lowerBound; i < upperBound; i++) {
if (i >= cachedAccountsIdsWithActivePositionsLength) break;
}
Impact
If the difference between upperBound and lowerBound is large, the function could consume excessive gas or even reach the block gas limit, preventing liquidations.
Recommended Mitigation Steps
Use here a maximum iteration limit to stop this:
uint256 constant MAX_ITERATIONS = 100;
for (uint256 i = lowerBound; i < upperBound && i < lowerBound + MAX_ITERATIONS; i++) {
if (i >= cachedAccountsIdsWithActivePositionsLength) break;
}
L-03: No use of pause mechanism in MarketOrderKeeper
Proof of Concept
MarketOrderKeeper.sol does not use pause mechanism, which could be crucial in emergency, un-predictable situations, given the context of the contract and its dynamic uses:
contract MarketOrderKeeper is ILogAutomation, IStreamsLookupCompatible, BaseKeeper {
}
Impact
In case of real time vulnerabilities, black hats do the wildest things, and/or in case of market anomalies, there's no way to quickly pause the system to prevent losses.
Recommended Mitigation Steps
Use a pausable pattern to be safe:
import "@openzeppelin/contracts/utils/Pausable.sol";
contract MarketOrderKeeper is ILogAutomation, IStreamsLookupCompatible, BaseKeeper, Pausable {
function fillMarketOrder(
uint128 tradingAccountId,
uint128 marketId,
bytes calldata priceData
)
external
onlyMarketOrderKeeper(marketId)
whenNotPaused
{
}
function pause() external onlyOwner {
_pause();
}
function unpause() external onlyOwner {
_unpause();
}
}
L-04: Centralization risk in configureSystemParameters
Proof of Concept
In GlobalConfigurationBranch.sol, the configureSystemParameters function allows immediate changes to critical system parameters:
function configureSystemParameters(
uint128 maxPositionsPerAccount,
uint128 marketOrderMinLifetime,
uint128 liquidationFeeUsdX18,
address marginCollateralRecipient,
address orderFeeRecipient,
address settlementFeeRecipient,
address liquidationFeeRecipient,
uint256 maxVerificationDelay
)
external
onlyOwner
{
}
Impact
Immediate changes to critical parameters by a single account poses ab obvious centralization risk and could disrupt the system if misused.
Recommended Mitigation Steps
Use a time-lock or multi-sig mechanism for critical parameter changes, make it more decentralised:
struct PendingConfig {
uint128 maxPositionsPerAccount;
uint128 marketOrderMinLifetime;
uint128 liquidationFeeUsdX18;
address marginCollateralRecipient;
address orderFeeRecipient;
address settlementFeeRecipient;
address liquidationFeeRecipient;
uint256 maxVerificationDelay;
uint256 effectiveTime;
}
PendingConfig public pendingConfig;
uint256 public constant CONFIG_DELAY = 2 days;
function proposeSystemParameters(
uint128 maxPositionsPerAccount,
uint128 marketOrderMinLifetime,
uint128 liquidationFeeUsdX18,
address marginCollateralRecipient,
address orderFeeRecipient,
address settlementFeeRecipient,
address liquidationFeeRecipient,
uint256 maxVerificationDelay
)
external
onlyOwner
{
pendingConfig = PendingConfig({
maxPositionsPerAccount: maxPositionsPerAccount,
marketOrderMinLifetime: marketOrderMinLifetime,
liquidationFeeUsdX18: liquidationFeeUsdX18,
marginCollateralRecipient: marginCollateralRecipient,
orderFeeRecipient: orderFeeRecipient,
settlementFeeRecipient: settlementFeeRecipient,
liquidationFeeRecipient: liquidationFeeRecipient,
maxVerificationDelay: maxVerificationDelay,
effectiveTime: block.timestamp + CONFIG_DELAY
});
}
function applySystemParameters() external onlyOwner {
require(block.timestamp >= pendingConfig.effectiveTime, "Config not ready");
delete pendingConfig;
}
L-05: Risk of DOS in liquidateAccounts due to gas limit
Proof of Concept
In LiquidationBranch.sol, the liquidateAccounts function processes all provided accounts in a single transaction:
function liquidateAccounts(uint128[] calldata accountsIds) external {
for (uint256 i; i < accountsIds.length; i++) {
}
}
Impact
If a large number of accounts need liquidation, the function might hit the block gas limit, preventing any liquidations from occurring.
Recommended Mitigation Steps
Use batched liquidations to better this:
uint256 constant MAX_LIQUIDATIONS_PER_BATCH = 10;
function liquidateAccounts(uint128[] calldata accountsIds) external {
uint256 endIndex = Math.min(accountsIds.length, MAX_LIQUIDATIONS_PER_BATCH);
for (uint256 i; i < endIndex; i++) {
}
}
L-06: No minimum order size check beyond zero in createMarketOrder
Proof of Concept
In OrderBranch.sol, the createMarketOrder function only checks for zero size, but does not enforce a minimum order size beyond that:
function createMarketOrder(CreateMarketOrderParams calldata params) external {
if (params.sizeDelta == 0) {
revert Errors.ZeroInput("sizeDelta");
}
}
Impact
While zero-sized orders are prevented currently, very small orders could still be created. This could then be used to spam the system or manipulate market data with minimal economic commitmentz.
Recommended Mitigation Steps
Use a minimum order size check:
uint128 internal constant MIN_ORDER_SIZE = 1e6;
import { Constants } from "@zaros/utils/Constants.sol";
function createMarketOrder(CreateMarketOrderParams calldata params) external {
if (params.sizeDelta == 0 || Math.abs(params.sizeDelta) < Constants.MIN_ORDER_SIZE) {
revert Errors.InvalidOrderSize(params.sizeDelta);
}
}
L-07: Risk of precision loss in getMarkPrice calculation
Proof of Concept
In PerpMarketBranch.sol, the getMarkPrice function performs division that could lead to precision loss:
function getMarkPrice(uint128 marketId, uint256 indexPrice, int256 skewDelta) external view returns (UD60x18) {
PerpMarket.Data storage perpMarket = PerpMarket.load(marketId);
return perpMarket.getMarkPrice(sd59x18(skewDelta), ud60x18(indexPrice));
}
Impact
Precision loss in price calculations could lead to small or large inaccuracies in order execution and position valuations.
Recommended Mitigation Steps
Use higher precision intermediates in calculations:
function getMarkPrice(uint128 marketId, uint256 indexPrice, int256 skewDelta) external view returns (UD60x18) {
PerpMarket.Data storage perpMarket = PerpMarket.load(marketId);
return perpMarket.getMarkPrice(sd59x18(skewDelta), ud60x18(indexPrice).scale(1e9));
}
L-08: No signature malleability protection in fillOffchainOrders
Recommended Mitigation Steps
Use EIP-2098 for signature compaction and malleability protection:
import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
function fillOffchainOrders(
uint128 marketId,
OffchainOrder.Data[] calldata offchainOrders,
bytes calldata priceData
)
external
onlyOffchainOrdersKeeper(marketId)
{
for (uint256 i; i < offchainOrders.length; i++) {
ctx.offchainOrder = offchainOrders[i];
bytes32 digest = _hashTypedDataV4(ctx.structHash);
ctx.signer = ECDSA.recover(digest, ctx.offchainOrder.signature);
}
}
Above method uses a single signature field instead of separate v, r, and s components, reducing gas and protecting against signature malleability.
L-09: Risk of front-running in createTradingAccount
Proof of Concept
In TradingAccountBranch.sol, the createTradingAccount function is susceptible to front-running:
function createTradingAccount(
bytes memory referralCode,
bool isCustomReferralCode
)
public
virtual
returns (uint128 tradingAccountId)
{
tradingAccountId = ++globalConfiguration.nextAccountId;
}
Impact
An attacker could game it and front-run account creation transactions to obtain specific account IDs, which might be desirable for some.
Recommended Mitigation Steps
Use the commit-reveal pattern for account creation:
mapping(address => bytes32) public accountCommitments;
uint256 public constant COMMITMENT_TIMEOUT = 1 hours;
function commitToAccountCreation(bytes32 commitment) external {
accountCommitments[msg.sender] = commitment;
}
function createTradingAccount(
bytes memory referralCode,
bool isCustomReferralCode,
bytes32 salt
)
public
virtual
returns (uint128 tradingAccountId)
{
bytes32 commitment = keccak256(abi.encodePacked(msg.sender, referralCode, isCustomReferralCode, salt));
require(accountCommitments[msg.sender] == commitment, "Invalid commitment");
require(block.timestamp <= accountCommitments[msg.sender] + COMMITMENT_TIMEOUT, "Commitment expired");
delete accountCommitments[msg.sender];
}
Above method requires users to commit to their account creation parameters before actually creating the account, making front-running much more difficult.
L-10: Not enough event emission for critical configuration changes
Proof of Concept
In GlobalConfiguration.sol, many configuration changes don't emit events:
function removeCollateralFromLiquidationPriority(Data storage self, address collateralType) internal {
}
Impact
No events logged for critical changes makes it difficult for off-chain systems to track and react to focused contract state changes.
Recommended Mitigation Steps
Use more events for all critical configuration changes:
event CollateralRemovedFromLiquidationPriority(address indexed collateralType);
function removeCollateralFromLiquidationPriority(Data storage self, address collateralType) internal {
emit CollateralRemovedFromLiquidationPriority(collateralType);
}
L-11: Risk of price manipulation in getPrice
Proof of Concept
In MarginCollateralConfiguration.sol, the getPrice function relies on a single price feed:
function getPrice(Data storage self) internal view returns (UD60x18 price) {
address priceFeed = self.priceFeed;
price = ChainlinkUtil.getPrice(
IAggregatorV3(priceFeed),
priceFeedHearbeatSeconds,
IAggregatorV3(sequencerUptimeFeed)
);
}
Impact
Reliance on a one single price feed could make the system vulnerable to price manipulations or temporary price feed failures in real time.
Recommended Mitigation Steps
A TWAP or median price mechanism using multiple price feeds, for cleaner and more precise price data:
function getPrice(Data storage self) internal view returns (UD60x18 price) {
address[] memory priceFeeds = self.priceFeeds;
require(priceFeeds.length >= 3, "Insufficient price feeds");
UD60x18[] memory prices = new UD60x18[](priceFeeds.length);
for (uint i = 0; i < priceFeeds.length; i++) {
prices[i] = ChainlinkUtil.getPrice(
IAggregatorV3(priceFeeds[i]),
self.priceFeedHearbeatSeconds,
IAggregatorV3(self.sequencerUptimeFeed)
);
}
return MedianLib.median(prices);
}
L-12: No slippage protection in getMarkPrice
Proof of Concept
In PerpMarket.sol, the getMarkPrice function doesn't include slippage protection:
function getMarkPrice(
Data storage self,
SD59x18 skewDelta,
UD60x18 indexPriceX18
)
internal
view
returns (UD60x18 markPrice)
{
return markPrice;
}
Impact
Large trades could significantly move the mark price, therefore enabling unfavourable executions for users.
Recommended Mitigation Steps
Put in slippage protection parameters:
function getMarkPrice(
Data storage self,
SD59x18 skewDelta,
UD60x18 indexPriceX18,
UD60x18 maxSlippage
)
internal
view
returns (UD60x18 markPrice)
{
UD60x18 maxAllowedPrice = indexPriceX18.mul(ud60x18(1e18).add(maxSlippage));
UD60x18 minAllowedPrice = indexPriceX18.mul(ud60x18(1e18).sub(maxSlippage));
require(markPrice.gte(minAllowedPrice) && markPrice.lte(maxAllowedPrice), "Price slippage too high");
return markPrice;
}
L-13: Risk of unexpected results with extremely large position sizes or prices in getNotionalValue
Proof of Concept
In Position.sol, the getNotionalValue function performs multiplication that could lead to unexpected results with extremely large values:
function getNotionalValue(Data storage self, UD60x18 price) internal view returns (UD60x18) {
return sd59x18(self.size).abs().intoUD60x18().mul(price);
}
Impact
Current solidity version in use prevents overflows, but extremely large position sizes or prices could still lead to wrong results due to the limitations of fixed-point arithmetic.
Recommended Mitigation Steps
Refine checks for extremely large values:
uint256 internal constant MAX_POSITION_SIZE = 1e27;
uint256 internal constant MAX_PRICE = 1e27;
import { Constants } from "@zaros/utils/Constants.sol";
function getNotionalValue(Data storage self, UD60x18 price) internal view returns (UD60x18) {
UD60x18 absSize = sd59x18(self.size).abs().intoUD60x18();
if (absSize.gt(ud60x18(Constants.MAX_POSITION_SIZE)) || price.gt(ud60x18(Constants.MAX_PRICE))) {
revert Errors.ExcessivePositionSize();
}
return absSize.mul(price);
}
L-14: Centralization risk in verifyDataStreamsReport
Proof of Concept
In SettlementConfiguration.sol, the verifyDataStreamsReport function relies on a single data source:
function verifyDataStreamsReport(
DataStreamsStrategy memory dataStreamsStrategy,
bytes memory signedReport
)
internal
returns (bytes memory verifiedReportData)
{
IVerifierProxy chainlinkVerifier = dataStreamsStrategy.chainlinkVerifier;
}
Impact
Reliance on a single data source could lead to system-wide issues if that source fails or is compromised.
Recommended Mitigation Steps
Use multiple data sources or a fallback mechanism:
function verifyDataStreamsReport(
DataStreamsStrategy memory dataStreamsStrategy,
bytes memory signedReport
)
internal
returns (bytes memory verifiedReportData)
{
for (uint i = 0; i < dataStreamsStrategy.verifiers.length; i++) {
try dataStreamsStrategy.verifiers[i].verifyReport(signedReport) returns (bytes memory data) {
return data;
} catch {
continue;
}
}
revert("All verifiers failed");
}
L-15: No gas limit for external calls in deductAccountMargin
Proof of Concept
In TradingAccount.sol, the deductAccountMargin function makes external calls without gas limits:
function deductAccountMargin(
Data storage self,
FeeRecipients.Data memory feeRecipients,
UD60x18 pnlUsdX18,
UD60x18 settlementFeeUsdX18,
UD60x18 orderFeeUsdX18
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
IERC20(collateralType).safeTransfer(recipient, amountToTransfer);
}
Impact
Without gas limits, external calls could lead to consuming all available gas, causing the transaction to revert and preventing margin deductions.
Recommended Mitigation Steps
Use gas limits for external calls:
import "@openzeppelin/contracts/utils/Address.sol;
function deductAccountMargin(
Data storage self,
FeeRecipients.Data memory feeRecipients,
UD60x18 pnlUsdX18,
UD60x18 settlementFeeUsdX18,
UD60x18 orderFeeUsdX18
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
Address.functionCallWithGas(
address(IERC20(collateralType)),
abi.encodeWithSelector(IERC20.transfer.selector, recipient, amountToTransfer),
50000
);
}
L-16: Risk of reentrancy in initializeRootUpgrade
Proof of Concept
In RootUpgrade.sol, the initializeRootUpgrade function makes external calls without clear and aggressive protection against reentrancy:
function initializeRootUpgrade(
RootProxy.BranchUpgrade[] memory,
address[] memory initializables,
bytes[] memory initializePayloads
)
internal
{
for (uint256 i; i < initializables.length; i++) {
address initializable = initializables[i];
bytes memory data = initializePayloads[i];
Address.functionDelegateCall(initializable, data);
}
}
Impact
If any of the initialized contracts have buggy code, they could possibly do re-entrancy attack during the initialization process.
Recommended Mitigation Steps
Be safe and add a reentrancy guard for the initialization process, for best practice:
import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
contract RootUpgrade is ReentrancyGuard {
function initializeRootUpgrade(
RootProxy.BranchUpgrade[] memory,
address[] memory initializables,
bytes[] memory initializePayloads
)
internal
nonReentrant
{
for (uint256 i; i < initializables.length; i++) {
address initializable = initializables[i];
bytes memory data = initializePayloads[i];
Address.functionDelegateCall(initializable, data);
}
}
}
