Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Q.A REPORTS

bigsam

L01-

PERFORMUPKEEP IN THE LIQUIDATION FUNCTION FAILS TO DECODE THE ADDRESS OF THE CONTRACT AND VERIFY ACCORDING

Summary

The `performUpkeep` function is currently not properly verifying the `performData` input. This function has been restricted by the `onlyForwarder` modifier, but the data within `performData` is not validated, leading to potential risks. To mitigate this, the `performData` should be decoded and validated to ensure it originates from the correct source.

Vulnerability Details

Based on the IAutomation interface-

/**
* @notice method that is actually executed by the upkeeps, via the registry.
* The data returned by the checkUpkeep simulation will be passed into
* this method to actually be executed.
@audit>> READ>> * @dev The input to this method should not be trusted, and the caller of the
* method should not even be restricted to any single registry. Anyone should
* be able call it, and the input should be validated, there is no guarantee
* that the data passed in is the performData returned from checkUpkeep. This
* could happen due to malicious upkeeps, racing upkeeps, or simply a state
* change while the performUpkeep transaction is waiting for confirmation.
@audit>> READ>> * Always validate the data passed in.
* @param performData is the data which was passed back from the checkData
* simulation. If it is encoded, it can easily be decoded into other types by
* calling `abi.decode`. This data should not be trusted, and should be
* validated against the contract's current state.
*/
function performUpkeep(bytes calldata performData) external;

Contracts are always advised to verify the data passed. Line 11 Always validate the data passed in.

But based on current implementation these data are not verfied

function checkUpkeep(bytes calldata checkData)
external
view
returns (bool upkeepNeeded, bytes memory performData)
{
---------------------------------------------------------
@audit>> Note>>> bytes memory extraData = abi.encode(accountsToBeLiquidated, address(this));
​
return (upkeepNeeded, extraData);
}

But when we use this data we fail to return address(this) and validate

function performUpkeep(bytes calldata peformData) external override onlyForwarder {
​
@audit >> DECODE 1 addess not 2,no verfication>>> uint128[] memory accountsToBeLiquidated = abi.decode(peformData, (uint128[]));
LiquidationKeeperStorage storage self = _getLiquidationKeeperStorage();
(IPerpsEngine perpsEngine) = (self.perpsEngine);
​
perpsEngine.liquidateAccounts(accountsToBeLiquidated);
}

Impact

This issue is classified as low severity due to the `onlyForwarder` restriction. However, failing to validate `performData` can still introduce potential risks, such as executing unintended liquidations.

Tools Used

- Solidity code analysis

- Review of the Chainlink Automation (Keepers) documentation

Recommendations

1. **Validate `performData`**: Decode `performData` and verify that it includes expected values, such as the correct contract address.

2. **Update `performUpkeep` Logic**: Modify the `performUpkeep` function to validate the decoded data before proceeding with any actions.

// Validate the origin address
function performUpkeep(bytes calldata peformData) external override onlyForwarder {
​
-- uint128[] memory accountsToBeLiquidated = abi.decode(peformData, (uint128[]));
​
++ // Decode the performData to extract the accounts and the address
++ (uint128[] memory accountsToBeLiquidated, address originAddress) = abi.decode(performData, (uint128[], address));
​
// Validate the origin address
++ require(originAddress == address(this), "Invalid performData: incorrect origin address");
LiquidationKeeperStorage storage self = _getLiquidationKeeperStorage();
(IPerpsEngine perpsEngine) = (self.perpsEngine);
​
perpsEngine.liquidateAccounts(accountsToBeLiquidated);
}

L02-

Nonce Management for Take Profit/Stop Limit Orders: Automation Required

Summary

The current system relies on users to manually increase the nonce when placing Take Profit (TP) or Stop Limit (SL) orders. According to the developer's intention, the nonce should be increased automatically when TP/SL are being filled. This report outlines the necessity for automating nonce increments for TP/SL orders to ensure seamless order execution.

Vulnerability Details

function fillOffchainOrders(
​
uint128 marketId,
​
OffchainOrder.Data\[] calldata offchainOrders,
​
bytes calldata priceData
​
)
​
external
​
onlyOffchainOrdersKeeper(marketId)
​
{
​
​
​
---------------------------------------------------------------------------------
​
​
// First we check if the nonce is valid, as a first measure to protect from replay attacks, according to
​
// the offchain order's type (each type may have its own business logic).
​
​
@audit>> kindly read>>> // e.g TP/SL must increase the nonce in order to prevent older limit orders from being filled.
​
​
// NOTE: Since the nonce isn't always increased, we also need to store the typed data hash containing the
​
// 256-bit salt value to fully prevent replay attacks.
​
if (ctx.offchainOrder.nonce != tradingAccount.nonce) {
​
revert Errors.InvalidSignedNonce(tradingAccount.nonce, ctx.offchainOrder.nonce);
​
}
​
​
​
​
​
----------------------------------------------------------------------------
​
​
// account state updates start here
​
​
// increase the trading account nonce if the order's flag is true.
​
​
@audit>> we relie on user's input>>> if (ctx.offchainOrder.shouldIncreaseNonce) {
​
unchecked {
​
tradingAccount.nonce++;
​
}
​
}
​

Nonce management is crucial in trading systems to ensure the correct sequence of order execution. A nonce is a unique number that increases with each order to prevent replay attacks and ensure that newer orders invalidate older ones. However, the current implementation requires users to manually increase the nonce for TP/SL orders, which can lead to the following issues:

1. **User Dependency**: Relying on users to manually increase the nonce introduces the risk of human error. Users may forget or incorrectly set the nonce, leading to unintended order executions.

2. **Inconsistent Order Execution**: If the nonce is not increased, older limit orders may still be filled/ although the structhash adds a added layer of protection to handle this, thus no issue here.

3. **Developer's Intent**: The developer's comments indicate that the nonce should be increased for every TP/SL order to prevent older limit orders from being filled. This should be automated to align with the intended functionality.

**Code Snippet and Practical Example:**

// e.g TP/SL must increase the nonce in order to prevent older limit orders from being filled
​
if (ctx.offchainOrder.shouldIncreaseNonce) {
unchecked {
tradingAccount.nonce++;
}
}
​

In the current implementation, the increaseNonce function should be called automatically whenever a TP/SL order is placed. This ensures that the nonce is correctly incremented, maintaining the correct order sequence.

Impact

Ensuring that nonce management is automated for these orders is crucial based on developer's intension.

Tools Used

- Manual Solidity code analysis

Recommendations

Modify the order placement logic to include an automatic nonce increment for TP/SL orders:

Check if an order reduces a position if it does we should increment nonce.

++ ///Check if an order reduces a position if it does we should increment nonce.
​
++ isIncreasing = Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta);
​
// account state updates start here
​
// increase the trading account nonce if the order's flag is true.
​
-- if (ctx.offchainOrder.shouldIncreaseNonce) {
​
++ if (ctx.offchainOrder.shouldIncreaseNonce||!isIncreasing) {
​
unchecked {
​
tradingAccount.nonce++;
​
}
​
}
​
​

L03-

Discrepancies in Long and Short Open Interest Calculation

Summary

This report addresses a discrepancy in the calculation of long and short open interest in the market. The current implementation may result in incorrect values for short open interest due to inconsistent handling of negative values, leading to potential inaccuracies in the display of open interest data.

Vulnerability Details

The function `getOpenInterest` calculates the long and short open interest for a given market. It involves computing the half skew, half open interest, and then deriving the long and short open interests. However, there is an inconsistency in how negative values are handled for long and short open interest.

**Code Snippet:**

/// @notice Returns the given market's open interest, including the size of longs and shorts.
​
/// @dev E.g: There is 500 ETH in long positions and 450 ETH in short positions, this function
​
/// should return UD60x18 longsOpenInterest = 500e18 and UD60x18 shortsOpenInterest = 450e18;
​
/// @param marketId The perps market id.
​
/// @return longsOpenInterest The open interest in long positions.
​
/// @return shortsOpenInterest The open interest in short positions.
​
/// @return totalOpenInterest The sum of longsOpenInterest and shortsOpenInterest.
​
function getOpenInterest(uint128 marketId)
​
external
​
view
​
returns (UD60x18 longsOpenInterest, UD60x18 shortsOpenInterest, UD60x18 totalOpenInterest)
​
{
​
// fetch storage slot for perp market
​
PerpMarket.Data storage perpMarket = PerpMarket.load(marketId);
​
​
// calculate half skew
​
SD59x18 halfSkew = sd59x18(perpMarket.skew).div(sd59x18(2e18));
​
​
// load current open interest from storage, convert to signed
​
SD59x18 currentOpenInterest = ud60x18(perpMarket.openInterest).intoSD59x18();
​
​
// calculate half current open interest
​
SD59x18 halfOpenInterest = currentOpenInterest.div(sd59x18(2e18));
​
​
// prepare outputs
​
@audit>> handling of long>>> longsOpenInterest = halfOpenInterest.add(halfSkew).lt(SD59x18\_ZERO)
​
? UD60x18\_ZERO
​
: halfOpenInterest.add(halfSkew).intoUD60x18();
​
​
@audit>> handling of short/ spilling above 0 before abs. not considered>>> shortsOpenInterest = unary(halfOpenInterest).add(halfSkew).abs().intoUD60x18();
​
totalOpenInterest = longsOpenInterest.add(shortsOpenInterest);
​
}
​
​

**Explanation:**

1. **Long Open Interest Calculation:**

longsOpenInterest = halfOpenInterest.add(halfSkew).lt(SD59x18\_ZERO)
​
? UD60x18\_ZERO
​
: halfOpenInterest.add(halfSkew).intoUD60x18();
​

- If `halfOpenInterest.add(halfSkew)` is less than zero, `longsOpenInterest` is set to zero.

- Otherwise, it is set to the calculated value.

2. **Short Open Interest Calculation:**

shortsOpenInterest = unary(halfOpenInterest).add(halfSkew).abs().intoUD60x18();

- Here, the calculation involves taking the absolute value, which can lead to non-zero values even when the input is positive already.

**Issue:**

- There is an inconsistency: when `halfOpenInterest.add(halfSkew)` is negative for long open interest, it is set to zero. However, for short open interest, the absolute value is taken, potentially resulting in incorrect values for values that are already positive.

Impact

The incorrect calculation of short open interest can lead to inaccurate data representation, which can mislead users and traders about the market conditions. This discrepancy can affect decision-making and risk assessment.

Tools Used

- Solidity code analysis

- Mathematical verification

Recommendations

1. **Consistent Handling of positive Values for short open interest:**

**Proposed Solution:**

function getOpenInterest(uint128 marketId)
​
external
​
view
​
returns (UD60x18 longsOpenInterest, UD60x18 shortsOpenInterest, UD60x18 totalOpenInterest)
​
{
​
​
----------------------------------------------------------------------------
​
longsOpenInterest = halfOpenInterest.add(halfSkew).lt(SD59x18\_ZERO)
​
? UD60x18\_ZERO
​
: halfOpenInterest.add(halfSkew).intoUD60x18();
​
​
-- shortsOpenInterest = unary(halfOpenInterest).add(halfSkew).abs().intoUD60x18();
​
​
++ shortsOpenInterest = unary(halfOpenInterest).add(halfSkew).gt(SD59x18_ZERO)
​
++ ? UD60x18_ZERO
​
++ : unary(halfOpenInterest).add(halfSkew) .abs().intoUD60x18();
​
​
totalOpenInterest = longsOpenInterest.add(shortsOpenInterest);
​
}
​

Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

Error is in decoding of `peformData`

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.