Kwenta 18

The whole purpose of the circuit is to proof that an account has generated a certain trading volume and fee over a specific period of time. The period is determined by the StartBlkNum and EndBlkNum given to the circuit as inputs.

The circuit calculates the associated volume and fee based on the following receipts:

1. Unclaimable receipts
2. ClaimableOrderFeeFlow receipts
3. ClaimableExecution receipts

It is important to check that the receipt’s blockNumber is within the valid range of the circuit’s StartBlkNum and EndBlkNum inputs, otherwise, volume and fee might be incorrectly calculated.

For unclaimable receipts, the following range check is done:

uint248.IsZero(uint248.IsLessThan(r.BlockNum, startBlk30DAgo)), // r.BlockNum >= startBlkOneMonthAgo   // @audit-info check 3: is in timerange
uint248.IsLessThan(r.BlockNum, c.StartBlkNum),                  // r.BlockNum < c.StartBlkNum

Reference: opv2_volume_fee_circuit.go#L82-L83

For claimableExecution receipts, the following range check is done:

uint248.AssertIsLessOrEqual(c.StartBlkNum, blockNumber) 
uint248.AssertIsLessOrEqual(blockNumber, c.EndBlkNum)

Reference: opv2_volume_fee_circuit.go#L147-L148

However, for the claimableOrderFeeFlow receipts, being used to calculate the fee amount, a appropriate range check is missing. This can be exploited to receive a higher fee rebate for the account. Invalid proofs can be generated by inserting claimableExecution receipts that don’t fall under the valid blocknumber range (startBlkNum <= blocknumber <= endBlkNum), allowing to add out-of-range claimableExecution receipts for the whole purpose of inflating the fee amount.

This kind of vulnerability can be often seen in ZK circuits and belongs to the category of “under-constrained” circuits. This essentially means that a malicious Prover can create an invalid proof which is subsequently successfully verified by the verifier. This happens as the circuit contains too few Constrains (aka checks).

Remediation to Consider

Add an appropriate blocknumber range check for the claimableOrderFeeFlow receipts, similar to what is done for the ClaimableExecution receipts: startBlkNum <= blocknumber <= endBlkNum

In FeeReimbursementClaim.claim() function, the rebate fee will be transferred to account, which is msg.sender. The function attempts to authenticate msg.sender by calling to smAccount.isAuth():

function claim(address _smartMarginAccount) external {
    address account = msg.sender;

    ...

    IAccount smAccount = IAccount(_smartMarginAccount);
    if (!smAccount.isAuth()) {
        revert Unauthorized();
    }

        ...

    rewardToken.transfer(account, feeRebateOP);

        ...
}

Reference: FeeReimbursementClaim.sol#L93-L121

However, the smAccount.isAuth() function is called by the FeeReimbursementClaim contract, which means msg.sender in the context of the isAuth() function is the FeeReimbursementClaim contract itself. As a result, the actual caller of FeeReimbursementClaim.claim() is not correctly authenticated, instead isAuth() attempts to authenticate the FeeReimbursementClaim contract address:

function isAuth() public view virtual returns (bool) {
    return (msg.sender == owner || delegates[msg.sender]);
}

Reference: Auth.sol#L54-L56

This will cause isAuth() reverting, so the owner of the smAccount can’t claim their rebate fee

Remediations to Consider

Consider checking authentication for the actual caller in the FeeReimbursementClaim.claim() function:

function claim(address _smartMarginAccount) external {
    address account = msg.sender;

    ...

    IAccount smAccount = IAccount(_smartMarginAccount);
-   if (!smAccount.isAuth()) {
+   if (!(smAccount.owner() == account) && !(smAccount.delegates(account)) ) {    
        revert Unauthorized();
    }

        ...
}

In the circuit implementation, a zero block number for claimable execution receipt is allowed. When its value is set to zero, it will be initialized to the value of StartBlkNum.

        claimableExecutionReceipts := sdk.RangeUnderlying(receipts, MaxReceipts-MaxClaimableTradesPerCircuit, MaxReceipts)
        // Makse sure claimable execution tx's block numbers are in ascending order
        sdk.AssertSorted(claimableExecutionReceipts, func(a, b sdk.Receipt) sdk.Uint248 {
            return uint248.Or(uint248.IsLessThan(a.BlockNum, b.BlockNum), uint248.IsEqual(a.BlockNum, b.BlockNum))
        })
        sdk.AssertEach(claimableExecutionReceipts, func(r sdk.Receipt) sdk.Uint248 {
=>		blockNumber := uint248.Select(uint248.IsZero(r.BlockNum), c.StartBlkNum, r.BlockNum)
            uint248.AssertIsLessOrEqual(c.StartBlkNum, blockNumber)
            uint248.AssertIsLessOrEqual(blockNumber, c.EndBlkNum)
        ...
        }

However, allowing claimableExecutionReceipts with a 0 block number may lead to unexpected results. For example, if one claimable execution receipt is provided with BlockNum value 0 and another with BlockNum equal to StartBlkNum, both will be processed, and their corresponding fees and volume will be accumulated even though they have identical other properties. If two claimable execution receipts were provided with identical properties, including BlockNum value, the circuit would revert due to the uniqueness assertion being invalidated.

As a result, allowing claimableExecutionReceipts with a 0 value for block number may circumvent input uniqueness constraint.

Remediations to Consider

Update circuit implementation to add proper constraints for the r.BlockNum.

In FeeReimbursementClaim’s _getChainlinkDataFeedLatestAnswer, a Chainlink price feed is used to retrieve the OP price:

(
    /*uint80 roundID*/
    ,
    int256 data,
    /*uint startedAt*/
    ,
    /*uint timeStamp*/
    ,
    /*uint80 answeredInRound*/
) = dataFeed.latestRoundData();

Reference: FeeReimbursementClaim.sol#L177-L188

However, the returned data is used “as is” without any further sanity checks.

It needs to be noticed, that the call to Chainlink’s price feed using latestRoundData() can return stale data in extreme situations, such as highly volatile market conditions and flash crashes. In such situations, the returned price can significantly deviate from the actual OP price.

Remediations to Consider

Follow best practices when consuming Chainlink price feeds. We have written a post about this.

In FeeReimbursementApp, the new claim period is calculated in _newClaimPeriod. The goal of the function is to create or extend the claim period with the provided startBlockNumber and endBlockNumber of the new claim period. Additionally, it is designed to prevent double-counting, meaning that the new period must not have any overlapping block numbers with the previous period.

For this to work, the current implementation relies on calling the handleProofResult function in the correct order. If this is not the case and the Brevis service is not calling the function in the correct order, claimPeriods might be left out and cannot be set at a later point in time anymore. Consider the following scenario:

1. A proof is generated for claimPeriod of
   • startBlockNumber: 1
   • endBlockNumber: 5
2. Next, a proof is generated for claimPeriod of
   • startBlockNumber: 8
   • endBlockNumber: 15

As a result, the claimPeriod is set to startBlockNumber: 1 and endBlockNumber: 15, implicitly assuming that the users is not eligible for any rebates for block numbers between 5 and 8.

However, a proof for block number 5-8 were not generated due to some outages of the Brevis Service. With the current design, resubmitting a proof for block number 5-8 will revert, effectively preventing the user from claiming any deserved rebates during this period.

Remediations to Consider

Adapt the logic in _newClaimPeriod accordingly so that it doesn’t need the Brevis service to generate the proofs in a consecutive order.

In FeeReimbursementClaim’s claim function, transfer is used to send the reward tokens to the user instead of using safeTransfer.

Some ERC20 tokens return false when the call fails instead of reverting. Depending on the rewardToken being used, this could cause claim to successfully run without the user acutal claiming the tokens.

Note that this issue is considered as low severity, since it is expected to only use OP ERC20 token as reward token. However, with the current implementation, any other ERC20 tokens could be specified as reward token.

Remediations to Consider

Use OpenZeppelin’s safeTransfer function.

In FeeReimbursementClaim’s _convertUSDtoOP function, the price data returned by the Chainlink price feed is expected to be 8 decimals, which is the case for the OP price feed.

It further scales up opPriceInWei to 18 decimals as follows:

uint256 opPriceInWei = uint256(price) * 1e10; 

Reference: FeeReimbursementClaim.sol#L142

The price is incorrectly calculated when using a price feed with decimals other than 8.

Note that this issue is considered low severity since it is expected to only use the OP price feed. However, with the current implementation, any other price feed could be provided at deployment.

Remediations to Consider

It is recommended to use the value returned by AggregatorV2V3Interface.decimals() to scale the price accordingly.

In FeeReimbursementApp, handleProofResult emits the following event:

emit FeeRebateAccumulated(
    account, feeRebate, volume30D, feeRebateWithRate, startBlockNumber, endBlockNumber
);

Reference: FeeReimbursementApp.sol#L94

The function is called by the Brevis service which passes an associated requestId. The requestId might be useful for tracking down any potential issues.

Remediations to Consider

Consider adding the provided _requestId to the FeeRebateAccumulated event.

FeeReimbursementApp uses revert to return custom errors as well as require statements to handle error messages. Additionally, FeeReimbursementClaim only uses custom errors.

Remediations to Consider

For consistency, consider replacing require statements with custom errors.

• In FeeReimbursementApp, rewardToken, rewardTokenDecimals and associated function setRewardToken is not used. Reference: FeeReimbursementApp.sol#L31-L32
• In FeeReimbursementApp, claimer and associated function setClaimer is not used. Reference: FeeReimbursementApp.sol#L34, FeeReimbursementApp.sol#L165
• In FeeReimbursementApp, the events MigrationDone and MigrationFinishedForAccount are not used. Reference: FeeReimbursementApp.sol#L55-L56
• In FeeReimbursementClaim, the defined error InsufficientContractBalance is not used. Reference: FeeReimbursementClaim.sol#L55
• In FeeReimbursementClaim, dataFeed and sequencerUptimeFeed can both be declared as immutable. Reference: FeeReimbursementClaim.sol#L23-L24
• In FeeReimbursementClaim, the _convertUSDtoOP specifies opAmount and price as “named return variables”. When using “named return variables”, values need to be assigned to the variables but there is no need to explicitly return the variables themselves. Reference: FeeReimbursementClaim.sol#L132
• In FeeReimbursementApp, the AlreadySet() event is defined but never used. Reference: FeeReimbursementApp.sol#L26
• In FeeReimbursementApp, the SafeERC20 import is never used. Consider removing unnecessary import and corresponding library usage using SafeERC20 for IERC20. Reference: FeeReimbursementApp.sol#L29

FeeModule contract inherits OZ’s Ownable functionality. However, none of that functionality is needed or used.

Consider removing unnecessary contract inheritance.

In FeeReimbursementApp, setClaimContract() is owner accessible function that can update the claim contract.

However, this important state change does not have corresponding event emission, which would facilitate off-chain tracking and monitoring.

Consider emitting appropriate event on state change.

In FeeReimbursementApp, handleProofResult() contains the calculation of feeRebate with externally retrieved rebateRate. The implementation assumes that the value of the percentage is within [0,100] and, as a result, uses a hardcoded 100 value as a denominator.

if (feeRebate > 0) {
    percentage = feeRebateTierModule.getFeeRebatePercentage(volume30D);
    feeRebateWithRate = feeRebate * percentage / 100;
}

However, if the percentage value was 1e18, as is commonly used to represent 100%, this hardcoded denominator would be incorrect, and consequently, the final result would be too.

Consider retrieving both percentage and precision (100) from the feeRebateTierModule or providing both volume30D and feeRebate to the feeRebateTierModule to calculate the final result.