Logium

CertiK16 Sep 2022Files named in the report

CertiK reviewed Logium on 16 Sep 2022: 26 findings, 0 critical; 4 marked fixed, 1 acknowledged, 21 open.

No open critical findings

Open the report Open Logium’s security record

Report hosted on skynet.certik.com.

Statuses are the security firm’s own, from this report. Trustblock does not review code.

Findings26

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

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	1	1
Medium	0	0	0
Low	21	0	3

LowAssembly UsageNot fixed

```solidity assembly { // isUp is decoded above calldatacopy(add(out, sub(0x40, 20)), add(self.offset, 1), 20) // pool let strike := sar(sub(256, 24), calldataload(add(self.offset, 21))) mstore(add(out, sub(0x60, 0x20)), strike) calldatacopy(add(out, sub(0x80, 4)), add(self.offset, 24), 4) // period let ratio := sar(sub(256, 24), calldataload(add(self.offset, 28))) mstore(add(out, sub(0xa0, 0x20)), ratio) calldatacopy(add(out, sub(0xc0, 4)), add(self.offset, 31), 4) // issuerWinFee calldatacopy(add(out, sub(0xe0, 4)), add(self.offset, 35), 4) // traderWinFee calldatacopy(add(out, sub(0x100, 4)), add(self.offset, 39), 4) // exerciseWindowDuration calldatacopy(add(out, sub(0x120, 20)), add(self.offset, 43), 20) // claimTo } ```

LowCode DuplicationNot fixed

The functions `totalStakeToPrice()` and `totalStake()` from the library `RatioMath` share the same calculation for the ration. The shared functionality can be placed into a separate `internal` or `private` function to improve the code's readability.

LowConfusing Error MessagesNot fixed

The linked `require` statements have an error message that doesn't reflect clearly what the problem is. The following suggestions are made to improve the code's readability: - "Can't allow allowed" can be changed to "Already allowed" - "Can't disallow disallowed" can be changed to "Already disallowed" - "Can't withdraw zero" can be changed to "Withdraw more than zero" - "Too low invalidation" can be changed to "Input a higher invalidation number"

LowDesign of Protocol Relies on Eip-712Not fixed

The contract `LogiumCore.sol` heavily depends on EIP-712 to interact with betting.

LowExternal EIP712 DependencyNot fixed

This contract relies on functionality that is off-chain to allow for withdrawal authorizations and to invalidate others.

LowFunction `multicallUnsafe()` Does Not DifferentiaNot fixed

The function `multicallUnsafe()` from contract `CallTool` returns the number of failures, but it does not tell which calls failed. This approach is not informative as the user needs to guess which transactions succeeded.

LowImproper Usage of `public` and `external` TypeNot fixed

`public` functions that are never called by the contract could be declared as `external`. `external` functions are more efficient than `public` functions.

LowInternal and Private Function Names Should StartNot fixed

The linked internal functions do not start with `_`. This makes it more difficult for developers/auditors to distinguish them from public or external functions, whose access restrictions need a more careful revision.

LowMissing Input Validation For `Details` StructureNot fixed

The function `unpackBinaryBetDetails()` from the library `TicketBinaryBet` does not perform any validation to the user supplied data `self`. The current documentation on the website and UI, we hope will help users protect themselves against such scenarios. However a malicious actor could mimic a website and create malicious bets on top of the main contract. The UI has to be correct for this to not be an issue. The user has to be very careful with how they interact with the protocol and make sure they are signing correct tickets and not malicious ones. The following scenarios can occur on a malicious phishing website: - set `issuersWinFee` or `traderWinFee` to zero. - change the `pool` address to an address controlled by the user. - change the `ratio` - set the address `claimTo` to an address they control.

LowMissing Zero Address ValidationNot fixed

The linked addresses are not checked before being used to make sure they are not the zero address. Addresses should be checked before assignment to make sure it is not zero addresses.

Show 10 more

LowMultiple Imports For The Same LibraryNot fixed

The library `ECDSA` from OpenZeppelin is imported twice in the contract `LogiumCore`.

LowPoor Parameter Naming `_0`Not fixed

The function's declaration for `users` on the interface `ILogiumCoreState` has an parameter of type `address` name "_0". This naming doesn't reflect what this parameter represents.

LowPotential Price Manipulation RiskNot fixed

The protocol only takes V3 Uniswap pool into account. If an attacker has enough funds to temporarily manipulate the price of the underlying asset, they could potentially liquidate others and win bets.

LowRedundant CheckNot fixed

The function `_withdrawFromTo()` from contract `LogiumCore` has a `require` statement that checks if the amount is less than the maximum value possible for the parameter. This check is redundant and can be deleted

LowRedundant Parameter DecodingNot fixed

The function `issue()` from contract `LogiumBinaryBet` receives the parameter `takeParas` which is of type `bytes32`. This parameter is then cast into the type `uint256` and is not further used.

LowReused Emit EventNot fixed

The event `CollateralChange` is emitted in four different functions that represent different scenarios, from deposits to withdraws. This technique is not helpful as it does not provide a clear message of what the function did. For example, for the function `depositTo()`, the event does not display who did the deposit to the target address.

LowThird Party DependenciesNot fixed

The contract is serving as the underlying entity to interact with third party Uniswap protocol. The scope of the audit treats 3rd party entities as black boxes and assume their functional correctness. However, in the real world, 3rd parties can be compromised and this may lead to lost or stolen assets. In addition, upgrades of 3rd parties can possibly create severe impacts, such as increasing fees of 3rd parties, migrating to new LP pools, etc. These phishing and DDOS attacks include: - Malicious DDOS against website to prevent user from using UI to exercise bet. - Malicious DDOS against bot to prevent bot from executing on chain. - If the gas fees are too high, the bot will not execute therefore site only needs to be DDOS. - Phishing website can use original contract and create malicious bets that have a 1 second `exerciseWindowDuration`. - Creating multiple bets on 1 contract with different amounts then DDOSing and hitting claim to take all funds. - Phishing attacks to get users signature for `WithdrawAuthorization`. This functionality can only be abused if done by strict malicious intent due to DDOSing or by failure of website and/or automated bot. The potential issue with the `claim()` function is considered a third party dependency due to the user having to trust the bot to execute the bet. If the bot does not execute the bet, then the user has to execute it themselves within the window to not automatically lose. Step 1. Wait for a contract to deploy. Step 2. Make another bet with same person on the exact same parameters. Step 3. `Hashval` should be the same and create another bet on top of it. Step 4. DDOS the bot for 5-15 minutes to be able to claim Step 5. Call claim after first expires and take all the funds in the contract. This works because the contract calls the balance of this in USDC meaning you can use funds that are not yours. ```solidity=145 uint256 balance = Constants.USDC.balanceOf(address(this)); require(balance > 0, "Nothing to claim"); (uint256 issuerCollateral, uint256 issuerWin) = RatioMath .totalStakeToPrice(balance, details.ratio); ``` By doing this even if my bet is $1, the balance should be equal to whatever the balance from another bet is. This is currently *not* an issue, but if the website had a DDOS attack or DDOS to the bot, but this could become an issue due to malicious actors.

LowUnchecked Value of ERC-20 `transfer()`/`transferFNot fixed

The aforementioned lines perform external call to `transferFrom` of ERC20 contracts and the return value is not checked in either case.

LowUnnecessary `abicoder v2` PragmaNot fixed

The linked statements explicitly declare the usage of `abicoder v2`. This feature is enabled by default in Solidity v0.8.0 and above. [Reference](https://docs.soliditylang.org/en/v0.8.13/080-breaking-changes.html)

LowUnused LibraryNot fixed

The library `ECDSA` is declared on the library `Ticket` but it's not used. It can be deleted to save gas.

Show 1 more

LowUse a Contract For Constant Instead of LibraryNot fixed

The project has two files with constants: `Constants.mainnet.sol` and `Constants.rinkeby.sol`. It is understandable that the used addresses change between the networks, but we would like to propose a different approach that consists on having a `Constants` contract with immutable parameters which will be passed as arguments. When deploying the contract, the arguments for the contract's constructor can change depending on which network they are deploying the project.