Opensky Finance

CertiK5 Sep 2022Files named in the report

CertiK reviewed OpenSky (Opensky Finance) on 5 Sep 2022: 45 findings, 1 critical; 35 marked fixed, 2 acknowledged, 8 open.

1 critical finding, fixed

Open the report Open OpenSky’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings45

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

Severity	Open	Acknowledged	Fixed
Critical	0	0	1
High	4	0	1
Medium	0	2	4
Low	4	0	29

CriticalAnyone Can Move Out All the Tokens via `claimERC2Fixed

`OpenSkyOToken` allows to deposit `_underlyingAsset` and get `oTokens` instead. However, anyone can move out all the tokens including `_underlyingAsset` to `_treasury()` via call to `claimERC20Rewards()`. This makes claiming of `oTokens` impossible.

High`outAmount` is refunded twice by `OpenSkyWETHGateFixed

`OpenSkyWETHGateway.extend()` refunds `outAmount` to `msg.sender` if positive. But `lendingPool.extend()` also transfers it directly to `newLoan.borrower` by the call to `ReserveLogic.extend()`. `newLoan.borrower` is set as `loanNFT.ownerOf(oldLoanId)` if `lendingPool.extend()` is called by `SETTINGS.wethGatewayAddress()`.

Medium`OpenSkyPunkGateway` doesn't `repay()` penaltyFixed

`OpenSkyPunkGateway.repay()` and `repayETH()` perform transfer to `address(this)` of `loanNFT.getBorrowBalance(loanId)`. This amount **doesn't include penalty**. `OpenSkyPunkGateway._repay()` then performs `approve()` of the same `borrowBalance` to `poolAddress()` and calls `poolAddress().repay()`. `OpenSkyPool.repay()` has `repayAmount = borrowBalance + penalty`. `ReserveLogic.repay()` is called with the same `amount` argument. It transfers tokens from `OpenSkyPunkGateway` to `reserve.oTokenAddress`. The transfer will fail since the amount approved is less then transferred.

Medium`OpenSkyWETHGateway` allows to `repay()` loans byFixed

If `OpenSkyWETHGateway` contract has non zero `WETH` balance for some reason, it can be used by anyone to repay the loans. `OpenSkyWETHGateway.repay()` can be called with zero `msg.value`. Infinite `WETH` was already approved to `lendingPool` and will be used by `lendingPool.repay()`. `extend()` is also affected.

Medium`supportsInterface()` calls itselfFixed

`OpenSkyLoan.supportsInterface()` calls itself instead of `super` version.

MediumUsage of `safeTransferFrom(address(this)` for ERCFixed

```solidity=170 IERC20(_underlyingAsset).safeTransferFrom(address(this), to, amount); ``` There is no need to use `safeTransferFrom()` with `this` as spender. Also, the contract has not approved itself to spend own tokens, the call will fail.

Low`basePriceData` can be declared as `storage` to sFixed

`basePriceData` structure is declared as `memory`. Several `nftPriceFeedMap` elements are fully copied from `storage` into `memory`.

Low`borrowETH()` emits both `Borrow` and `BorrowETH`Fixed

`borrowETH()` emits `Borrow` and `BorrowETH` events. `repayETH()` emits `Repay` and `RepayETH` events.

Low`check msg.value` comment is misleadingFixed

`check msg.value` comment is misleading. There is no `msg.value`.

Low`MONEY_MARKET_DEPOSIT_AMOUNT_ALLOWED` name is misFixed

`MONEY_MARKET_DEPOSIT_AMOUNT_ALLOWED` error name is supposed to be `MONEY_MARKET_DEPOSIT_AMOUNT_NOT_ALLOWED`.

Show 10 more

Low`OpenSkyWETHGateway.constructor()` comment is misFixed

The comment states `Infinite approves OpenSky pool`, however, it doesn't. Separate functions `authorizeLendingPool()` and `authorizeLendPoolNFT()` are responsible for that.

Low`Repay` event should emit `nftReceiver` addressFixed

`IOpenSkyPool Repay` event has argument `onBehalfOf The address that will receive the pledged NFT`. However, `onBehalfOf` is emitted instead of `nftReceiver`.

LowChecking if `amount > 0` is uselessFixed

```solidity=140 uint256[] memory ids = new uint256[](amount > 0 ? amount : 0); if (amount > 0) { for (uint256 i = 0; i < amount; ++i) { ``` `amount > 0` checks are redundant, don't affect the functionality.

LowCommented Out CodeFixed

The linked lines have commented out code that is redundant and can be deleted.

LowContract Size Exceeds LimitFixed

The linked contracts have a code size that exceeds 24576 bytes, so they may not be deployable on the mainnet.

LowError message `DEPOSIT_PUNK_NOT_OWNER_OF_PUNK` isFixed

Error message `DEPOSIT_PUNK_NOT_OWNER_OF_PUNK` is supposed to be `BORROW_NOT_OWNER_OF_PUNK`

LowImproper Usage of `public` and `external` TypeFixed

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

LowInconsistent Comment and CodeFixed

The comment above the function `endLiquidation()` from contract `ReserveLogic` states "Implements the start liquidation feature". There is an inconsistency issue with the "start" and "end" wording. The comment on the contract `OpenSkyInterestRateStrategy` references the value `OPTIMAL_USAGE_RATIO` which is never used, while the code does reference a value `OPTIMAL_UTILIZATION_RATE`.

LowInconsistent Contract File NamingFixed

The file `interfaces/IOpenSkyMoneymarket.sol` has 'm' in lowercase. But it is imported as `IOpenSkyMoneyMarket.sol`. This can cause a problem on [different file systems](https://docs.soliditylang.org/en/v0.8.13/path-resolution.html#import-callback).

LowInconsistent Functionality NamingFixed

Along with the project's codebase, there are references to Flash Loans, but the functionality that resembles that logic is called "FlashClaim".

Show 10 more

LowIncorrect comments and argument names of `ReserveFixed

```solidity=119 * @param ethIn The amount of ETH that will be deposited * @param ethOut The amount of ETH that will be withdrawn ``` `extend()` doesn't work with Ether, it works with any underlying ERC20 token.

LowMissing `checkReserveExists` modifierFixed

Functions working with `reserves` should check if reserve exists.

LowMissing Emit EventsFixed

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

LowMissing Error MessagesFixed

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.

LowMissing Zero Address Validation on OpenSkyPriceAgFixed

The linked addresses are not checked before being used to make sure they are not the zero address.

LowPotential Reentrancy AttackFixed

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. A user can create a malicious contract that calls `flashClaim()` and that in its implementation of the function `executeOperation()`, the contract calls the function `flashClaim()` again, starting a new flash loan with different parameters. This could allow a malicious user to start a series of malicious transactions involving different smart contracts, as the new calls to `flashClaim()` can point to different `receiver` addresses.

LowThird Party DependenciesFixed

The contract is serving as the underlying entity to interact with third party `Aave` and `Uniswap` protocols. 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.

LowTyposFixed

There are several typos in the code's comments. - aidrop should be airdrop. - backword should be backward. - computated should be computed. - curreBalanceScaled should be currentBalanceScaled. - excuted should be executed. - OWERFLOW should be OVERFLOW. - stucked should be stuck. - SUFFIENCE should be SUFFICIENT. - "The rate will be set" should be "The rate to be set".

LowUnchecked Value of ERC-20 `transfer()`/`transferFFixed

The linked `transfer()`/`transferFrom()` invocations do not check the return value of the function call which should yield a `true` result in case of a proper ERC-20 implementation.

LowUnnecessary ABIEncoderV2 PragmaFixed

The linked line explicitly specifies `pragma ABIEncoderV2`, which is unnecessary due to the feature being enabled by default in Solidity v0.8.0 and above.

Show 5 more

LowUnnecessary Usage of SafeMath LibraryFixed

For Solidity version 0.8.x, the SafeMath library is no longer needed because native math operations are already safe and cheaper.

LowUnused Import FilesFixed

The linked `import` statements include a contract or library which is not used in the file that imports such files.

LowUnused Return ValueFixed

The return value of the linked external calls are not used. This can cause unexpected errors.

LowUsage of `msg.sender` instead of `_msgSender()`Fixed

The function `withdraw()` from contract `OpenSkyPool` uses `msg.sender` to retrieve the sender of the transaction instead of using the function `_msgSender()`. Given that the function is inherited from the contract `Context`, the return value will be the same, but there will be consistency across the contract.

LowVariables That Could Be Declared as ImmutableFixed

The linked variables assigned in the constructor can be declared as `immutable`. Immutable state variables can be assigned during contract creation but will remain constant throughout the lifetime of a deployed contract. A big advantage of immutable variables is that reading them is significantly cheaper than reading from regular state variables since they will not be stored in storage.