OptionBlitz

CertiK6 Sep 2023Files named in the report

Audit

CertiK reviewed OptionBlitz on 6 Sep 2023: 53 findings, 1 critical; 36 marked fixed, 1 acknowledged, 16 open.

1 critical finding, fixed

Open the report Open OptionBlitz’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings53

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	1
High	0	1	5
Medium	0	0	7
Low	16	0	23

CriticalLack of Restriction on `HistoryVolatility.setWindFixed

In the contract `HistoryVolatility`, the function `setWindowSize()` can be called by anyone to set `windowSize` to any value that is less or equal to `0x7FFFFFFFFFFFFFFF`. \ This could be used by a malicious user to interfere with the contract logic, since all pricing functions rely on a volatility parameter which is computed using `windowSize`.

High`withdraw()` Burns Users Tokens As Opposed To TreFixed

The function `withdraw()` allows users to withdraw their stake, however, if they unstake `BLX` then a portion of it will be burned and the remaining portion returned to the user. The function `_splitWithdraw()` is called on the `blxAmount` to unstake which seperates the `blxAmount` into `toBurn`, the amount of `BLX` that should be burned, and `toUser`, the amount that should be returned to the user. Then `treasury.withdrawBlx(_msgSender(), toUser, toBurn)` is called, which has the following logic: ```solidity function withdrawBlx(address user, uint withdrawAmount, uint burnAmount) external override onlyTrustedCaller { if (withdrawAmount > 0) { _payTokensTo(BLX, user, withdrawAmount); } if (burnAmount > 0) { //there is no special function for burn //BLX.burnFor(user, burnAmount); // we transfer user's balance to burner // and burner would periodically burn them // on L2, it would mean transfer back to L1(by the burner) // then burn there // user must give treasury enough allowance for the transferFrom call BLX.transferFrom(user, burner, burnAmount); emit BlxBurned(user, burnAmount); } } ``` Which will transfer the `toUser` amount of `BLX` to the user. However, the `toBurn` amount will also be burned from the user, when it should be burned from the treasury. This results in the user receiving less tokens than intended.

HighIncorrect Rewards After Lock With Burn Period EndFixed

In the function `getUserPercents()`, it has logic to determine what status an account should be. If a user calls `lockWithBurn()` and the lock ends, then the client stated it should have the status of USDC only with no lock. However, if `deposit()`, `applyGainLoss()`, or `claimReward()` is called after the lock ends it will have the status of USDC and BLX with no lock because these functions will not set the users `burnedBlxAmount` to zero after the lock ends. We are unsure of the design intent for payout increases with BLX burns. If the intent is to have the payout increase to only exist during the lock, then there is the following issue: The function `calculatePayoutIncrease()` will not calculate the correct payout increase if a user calls `lockWithBurn()`, the lock ends, and the user calls `deposit()`, `applyGainLoss()`, or `claimReward()`. This is because these functions will not set the users `burnedBlxAmount` to zero after the lock ends.

HighLoss Offset Is Accounted For Twice In Users StakeFixed

The function `withdraw()` updates the `userStakes[sender].usdAmount` to be the `userStakes[sender].usdAmount` after gains and losses are applied minus the amount of USD to be withdrawn plus the amount of loss that was offset by rewards. However, `_applyGainLoss()` does the following: ```solidity userStakes[_account].usdAmount -= loss - offset; ``` which accounts for the loss that was offset by rewards. Thus it is accounted for twice and will cause `userStakes[_account].usdAmount` to be larger than it should be so that funds are not properly allocated. This can lead to a lack of funds in the treasury to cover users stakes and rewards. In addition, it cause potential issues for users when they attempt to withdraw their entire stake when `minUSDAmount` is nonzero as it requires them to account for the offset.

HighNot Possible To Only Deposit `USD` If `minBlxAmouFixed

The staking contract has percentages for locking `USDC` or locking both `USDC` and `BLX`. In addition, there are values `minBlxAmount` and `minUsdAmount` that give a minimum amount that must be staked or remain staked when withdrawing. However, in the function `deposit()` the following check is made: ```solidity require(newBlxAmount >= minBlxAmount, "SC:BLX_MIN_AMOUNT_FAIL"); ``` Where, `newBlxAmount` will be the total amount of `BLX` that would be staked including any amount previously staked. However, if a user stakes only `USDC`, then this amount will be zero causing the check to fail if `minBlxAmount` is nonzero.

HighUsers Will Not Receive An Increased Payout For BuFixed

Both functions `lock()` and `lockWithBurn()` are designed to lock a users stake for the input `duration`. The difference being `lock()` simply locks the `BLX`, while `lockWithBurn()` burns `BLX` in exchange for a higher multiplier when calculating the payout. However, the function `lockWithBurn()` does not update the `burnedBlxAmount` of the stake. As such the `burnedBlxAmount` will always be zero so that the increase calculated for burning `BLX` will always be zero. With the current implementation it will cause both `lock()` and `lockWithBurn()` to give the same payout.

Medium`lockWithBurn()` Burns BLX From User Instead Of TFixed

The function `lockWithBurn()` is designed to lock a user's stake and burn `BLX` in order to get an increased multiplier when calculating a user's payoff. When a user stakes with USDC and BLX, their `blxAmount` is held by the treasury. Then when a user calls `lockWithBurn()` it will call `treasury.withdrawBlx(account, 0, info.blxAmount)` which will then burn the amount of BLX staked from the user as opposed to burning it from the treasury which holds the staked amount. Thus in total, the user will have used `2 * blxAmount`, half of which is still staked in the treasury and the other half of which is burned.

Medium`setMinUsdAmount()` Updates `minBlxAmount` insteaFixed

The function `setMinUsdAmount()` should change the `minUsdAmount`, however, instead the function updates `minBlxAmount`.

MediumInconsistencies Between `_bet_close()` FunctionsFixed

In the option contracts, the function `_bet_close()` is supposed to: - pay the trader - update the stats of the trader - update the changes of balance in the treasury - unlock the collaterals that have been locked during the opening of the bet However, the conditions under which those steps are taken and the orders differ between the contracts: #### In the contracts `OptionBinary`, `DoubleNoTouch`, `OptionNoTouch`, : Only if the bet is won: 1. pay the trader Always: 2. update the stats of the trader 3. unlock the collaterals that have been locked during the opening of the bet 4. update the changes of balance in the treasury #### In the contract `DoubleTouch`: Only if the bet is won: 1. unlock the collaterals, 2. pay the trader 3. update the stats of the trader Always: 4. update the changes of balance in the treasury #### In the contract `OptionTouch`: Only if the bet is won: 1. pay the trader 2. update the stats of the trader Always: 3. update the changes of balance in the treasury 4. unlock the collaterals #### In the contract `AmericanOption`: The stats of the trader are updated when opening a bet and not when closing it. For the three other steps: Only if the bet is won: 1. pay the trader Always: 2. unlock the collaterals 3. update the changes of balance in the treasury Those inconsistencies could interfere with the logic of the other contracts, for example, the treasury relies on the amount of collateral locked to allow specific transfers.

MediumInconsistency in the volume updatesFixed

The call to `affiliate.updateVolume()` has been removed from all `_bet_add()` functions except in `DoubleTouch` and `OptionTouch` contracts. \ `affiliate.updateVolume()` increases, for a double touch and a touch option: - `stats[user][token].investmentDigital` - `stats[referer][token].refInvestmentDigital` if there is any referer when adding a bet. However, those values are also increased by `affiliate.updateVolume()` which is called when closing a bet in all the option contracts. \ This implies that users winning double touch and touch options will have their stats increased twice compared to the other types of options. This could lead to inconsistency when keeping a record of the volume invested and especially when users claim rewards since `calculateReward()` rely on `stats[user][token]` to compute the amount a user can claim.

Show 10 more

MediumLogical Issue in `Affiliate.updateStats()`Fixed

In the contract `Affiliate`, the function `updateStats()` has a specific block of code executed only if `isDigital` is false and `token == BLX`. \ If the fees are paid using `USDC`, the stats: `stats[user][USDC].investmentClassic` and `stats[referer][USDC].refInvestmentClassic` won't be correctly updated. This could cause inconsistency in the stats of a user.

MediumPotential Oracle ManipulationFixed

The function `getBlxUsdRate()` returns the current price according to the `UniswapV3Pool`. As the price depends on the single pool it can be manipulated by users with a large amount of funds or through flash loans. A malicious user may manipulate the price to do the following: - increase the price of BLX before they lock, to require less Blx tokens to get the maximum increase; - increase the price of BLX before they claim rewards if they did not lock a stake to get the maximum increase; - increase the price of BLX before paying commission, to pay less BLX as commission;

MediumUnsafe TypeCastingFixed

In the function `OptionAmerican.bet_add()`, the duration of an option is given as the uint256 parameter `expiry`. This parameter is unsafely cast to an `uint16`, this could potentially allow overflow allowing someone to place an option with a duration lower than the `MIN_DURATION`.

Low`StakingRewards` Can Be Removed Or SimplifiedFixed

Much of the functionality of the contract `StakingRewards` is unused and can be removed. The lines reference some of these instances, but not all of them and we recommend revisiting each component of the code to determine if it is necessary. In particular, the functions `_stake()`, `_withdraw()`, and `_getReward()` are commented out in the contract `StakingRewards` and they are disabled in the contract `StakingContract`. As these functions are never used they can be removed from the codebase and interface.

LowDiscrepancy with the mathematical modelFixed

In the contract `Treasury`, the function `get_r2()` computes r2: $R2 = \frac{totalCollateral(product,\ derivativeId)}{totalStake}$ with `totalCollateral()` returning the total collateral value: ```solidity=374 /// @return total collateral value ``` However in the **Product specifications**, r2 is suppposed to be: $R2 = \frac{Value \ of \ collateral \ for \ all \ the \ live \ options \ \cdot \ MCRU}{PoolNetValue}$. ------ Moreover, in the **Product specifications**, R1 is supposed to be computed using *Value of collateral for all options* and R2 with *Value of collateral for all the live options* but they both use `totalCollateral()`.

LowFunction Can Be Made Public To Avoid External CalFixed

The function `_updateBalance` has the following code: ```solidity this.distributeGainLoss(); ``` Which makes an external call to the contract itself to call `distributeGainLoss()` as it is an external function. However, if `distributeGainLoss()` was made public, then it can call the function directly without making an external call.

LowGains And Loses Should Be Distributed Before UserFixed

When users lock a stake they receive an increased multiplier and the price of BLX is locked. However, if there is a gain and a user is entitled to a payout before their lock ends, but `_applyGainLoss()` is not called until after the lock ends for their account, then they will not receive the increased multiplier.

LowInaccurate Function CommentFixed

It appears that the comment associated with the function `OptionAmerican._bet_add()` may not align with its actual functionality. Consider revising or removing the comment to accurately reflect the function's purpose and operation.

LowInvalid Check in `DoubleTouch.calculateOptionPricFixed

In the function `DoubleTouch.calculateOptionPrice()`, `priceOpen`, `upperBarrier`, and `lowerBarrier` could be equal. In this case, the call to `formulas.option_double_touch()` will fail.

LowLeftover Platform Beneficiary Income Is Not HandlFixed

The `platformOwnIncome` is stored via the beneficiaries address. Thus if a beneficiary has unclaimed income and calls `setPlatformBeneficiary()` then they will no longer be able to claim their pending income unless they are made the beneficiary again. In particular, the beneficiaries gap will go unclaimed.

Show 10 more

LowMay Withdraw USD That Is LockedFixed

The function `withdrawPlatformOwnIncome()` does not check that contract will still hold the amount of `lockedCollateral` if the `reward` token is USD. Similarly the function `payTokensTo()` does does not check that contract will still hold the amount of `lockedCollateral` if the trusted caller is not `staking`.

LowMissing Input ValidationFixed

In the contract `OptionBase`, the deployer should provide two `uint256` when calling the constructor; those will be used to set the two state variables: `MIN_DURATION` and `MAX_DURATION`. However, there are no checks to prevent wrong values to be set, in particular is should be checked that `MIN_DURATION >= MAX_DURATION`.

LowMissing Zero Address ValidationFixed

Addresses are not validated before assignment or external calls, potentially allowing the use of zero addresses and leading to unexpected behavior or vulnerabilities. For example, transferring tokens to a zero address can result in a permanent loss of those tokens. ```solidity=35 l1BlxToken = _l1BlxToken; ``` - `_l1BlxToken` is not zero-checked before being used. --- ```solidity=36 l2GatewayRouter = _l2GatewayRouter; ``` - `_l2GatewayRouter` is not zero-checked before being used. --- ```solidity=37 l1Burner = _l1Burner; ``` - `_l1Burner` is not zero-checked before being used. --- ```solidity=24 rewardsDistribution = _rewardsDistribution; ``` - `_rewardsDistribution` is not zero-checked before being used. --- ```solidity=84 stakingContractAddress = _stakingContractAddress; ``` - `_stakingContractAddress` is not zero-checked before being used.

LowModification on `OptionAmerican.payCommission()`Fixed

The function `OptionAmerican.payCommission()` has been modified to take two new parameters: - `duration` - `volume` The purposes of these changes seem unclear, for example: `Affiliate.updateStats()` is now called with the parameter `volume` instead of the `amount` as before which change significantly the logic of the function. Moreover, the new logic of `payCommission()` is not described in any documentation you previously provided.

LowMultiple *if* BlocksFixed

In the contract `Affiliate`, the function `claimReward()` checks that the token used is either `USDC` or `BLX` throught the following **require** statement: ```solidity=431 require(token == USDC || token == BLX, "AFF:TOKEN NOT SUPPORTED"); ``` The functions use then two distinct *if* blocks as follows: ```solidity if(token == USDC) { ...... } if(token == BLX) { ...... } ``` The second *if* statement is unnecessary since if `token != USDC`, the previous **require** statement implies that `token == BLX`.

LowPossible Improper Accounting If `minUsdAmount` IsFixed

If `minUsdAmount = 0`, then it is possible to successfully call `deposit(0,0)`. Which will not deposit any new funds, but will distribute the gains and losses. However, `StakingSnapshot` decreases the `totalUSDDeposits` when a loss is added, but does not increase the `totalUSDDeposits` when rewards are added. As the rewards are allowed to offset the losses, the amount of losses that are offset then need to be added back to the `totalUSDDeposits` plus any additional funds being deposited. This is done in the following logic in `deposit()`: ```solidity if (usdAmount > 0) { treasury.takeTokensFrom(sender, usdAmount); stakingSnapshot.increaseTotalUSDDeposits(usdAmount + lossOffset); } ``` However, it is only executed when `usdAmount > 0`. Thus if `deposit(0,0)` can successfully be called, then this logic will not be executed. If there is a nonzero `lossOffset` when distributing the rewards this will cause the `totalUSDDeposits` in `StakingSnaphsot` to be incorrect.

LowRedundant CheckFixed

`_depositLoss` is already checked to be nonzero in the modifier.

LowRedundant Or Unused of ModifiersFixed

In the contract `Treasury`, the internal function `_updateBalance()` is only called once by `registerBalanceChange()`. \ Both of these functions call the modifiers: ```solidity onlyValidToken(token) onlyValidProduct(product) ``` --- In the contract `StakingContract`, the modifier `updateLastDistribution` is not used. In the contract `Treasury`, the modifier `updateLastDistribution` is not used, however it is commented out as a modifier in `distributeGainLoss`.

LowRepeat Operation Can Be Stored To Save GasFixed

`loss-offset` is used multiple times in the logic. Thus one can store it in memory and reference it as opposed to calculating it multiple times to save gas.

LowTautologyFixed

The linked statement requires a `uint256` variable to be greater than or equal to 0. This statement will always return `true` because unsigned integers cannot be less than 0.

Show 6 more

LowToo Many DigitsFixed

Literals with many digits are difficult to read and review. ```solidity=21 uint public constant PCT_BASE = 100000; ``` --- ```solidity=104 uint24[5] memory classicTiers = [600000, 700000, 800000, 900000, 1000000]; ``` --- ```solidity=114 uint24[5] memory selfClassicTiers = [0, 100000, 250000, 500000, 500000]; ```

LowTypoFixed

In the contract `Treasury`, `tokeType` is used instead of `tokenType`.

LowUnresolved `FIXME` CommentFixed

The linked statement is a `FIXME` comment. Such comments are used to identify unimplemented code logic.

LowUnused Parameter in `american_price_call_put()`Fixed

In the function `Formulas.american_price_call_put()`, the parameter `N_lot` is declared but never used in the computation. \ The `N_lot` parameter seems to indicate that this function was supposed to compute $N_{lot} * | S_0 * (\phi(\frac{hv}{2}) - \phi(\frac{-hv}{2})) | $ but is really computing $S_0 * (\phi(\frac{hv}{2}) - \phi(\frac{-hv}{2})) $. Moreover, the comments on `Formulas.american_price_call_put()`, `Formulas.american_collateral_call_accurate()` and `Formulas.american_collateral_put_accurate()` indicate that the volatility parameter should be multiplied by a profit factor: ```solidity /// @param hv volatility value scaled to duration and multiplied by profit factor ``` ```solidity /// @param hv volatility value scaled to duration and profit margin ``` However, none of the functions calling these seem to provide a volatility value multiplied by another factor.

LowUnused Parameter in `looseRewards()`Fixed

In the contract `Affiliate`, the function `looseRewards()` takes a parameter `uint usdValue` but never uses it. \ Hence, in the function `_checkMonthPassed()`, the purpose of this line regarding `USDC`: ```solidity=55 looseRewards(accruedRewards[user][USDC], accruedRewards[user][BLX]); ``` is unclear. --- In the contract `Treasury`, there is a function `onLooseRewards` with a `TODO` comment on how to use the lost rewards. This may be related.

LowUnused Variables And Struct ElementsFixed

The variables `rewardPerUnitStake` and `lossPerUnitStake` are never updated and will always be 0. Also then `StakeInfo.rewardPerUnitStake` and `StakeInfo.lossPerUnitStake` are unnecessary as they are only ever made these variables which will be zero.