✅Opyn Gamma - OpenZeppelin
Progress
[H-1] ETH could get trapped in the protocol
The [Controller contract](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/Controller.sol#L27) allows users to send arbitrary actions such as possible flash loans through the [_call internal function](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/Controller.sol#L763).
Among other features, it allows sending ETH with the action to then perform a call to a [CalleeInterface type of contract](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/interfaces/CalleeInterface.sol#L9).
To do so, it saves the original msg.value sent with the [operate function call](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/Controller.sol#L331) in the [ethLeft variable](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/Controller.sol#L460) and it updates the remaining ETH left after each one of those calls to revert in case that it is not enough.
Nevertheless, if the user sends more than the necessary ETH for the batch of actions, the remaining ETH (stored in the ethLeft variable after the last iteration) will not be returned to the user and will be locked in the contract due to the lack of a withdrawEth function.
Consider either returning all the remaining ETH to the user or creating a function that allows the user to collect the remaining ETH after performing a Call action type, taking into account that sending ETH with a push method may trigger the fallback function on the caller’s address.
Update: Fixed in PR#304 where the payable property is removed from the operate function. However this change also means it is impossible to do outbound calls which require ETH through the operate function.
[C-2] Protocol could become insolvent due to market’s natural movements
The Protocol uses 2 time-based prices to value all the actions related to assets: live and expired prices.
The live prices are used by the [getExcessCollateral function](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/MarginCalculator.sol#L74) from the MarginCalculator contract to calculate the margin of a vault before the oToken expires. After the oToken expires, the expired price is then used to calculate the vault margin.
Because the collateral asset may not be the same as the strike asset or the underlying asset, it is not guaranteed that the collateral’s expiration price will be higher than its price in any point in the past, when the oToken has not expired yet, resulting in a possible undercollateralized situation for vaults.
Even though collateral assets must be whitelisted to be able to be used in the platform, with the caveat described in the issue oToken can be created with a non-whitelisted collateral asset, assets such as cTokens from Compound that are supposed to gain value over time may suffer a drop in their values due to market fluctuations or events as the one in early 2020, which produced a drop in value for cDai token. When the value of all collateral is worth less than the value of all borrowed assets, we say a market is insolvent. In case the platform allows the usage of non-monotonically-increasing price assets, the insolvency may be caused by a simple market price fluctuation.
Here are other examples of things that could cause a market to become insolvent:
The price of the underlying (or borrowed) asset makes a big, quick move during a time of high network congestion.
The price oracle temporarily goes offline during a time of high market volatility. This could result in the oracle not updating the asset prices until after the market has become insolvent.
The admin or oracle steals enough collateral that the market becomes insolvent.
Administrators list an ERC20 token with a later-discovered bug that allows minting of arbitrarily many tokens. This bad token is used as collateral to borrow funds that it never intends to repay.
In any case, the effects of an insolvent market could be disastrous. It may result in a “run on the bank” situation, with the last suppliers out losing their money. It is important to know that this risk does exist and it can be difficult to recover from even a small dip into insolvency.
Although the [AddressBook contract includes a slot for a liquidator manager](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/AddressBook.sol#L27), the system currently does not have any liquidation process, any vault is susceptible to becoming insolvent during the payout of the options right after expiration.
Consider adding a liquidation process to prevent insolvent vaults, carefully selecting the whitelisted assets for the protocol, adding more test units, and running a testnet version to understand how other assets may cause an undercollateralized scenario.
Update: Fixed in PR355, the Opyn team restricted all oTokens issued on the protocol to be collateralized with the exact payout asset depending on the oToken type(strike asset for PUT options and underlying asset for CALL options). Although this update restricted what Options the Opyn protocol can issue, it eliminated the risk of protocol become insolvent. We suggest a liquidation component to be ready in the future should the Opyn Team decide to remove PR 355 restriction, and get it thoroughly audited.
[M-1] oToken can be created with a non-whitelisted collateral asset
A product consists of a set of assets and an option type. Each product has to be whitelisted by the admin using the whitelistProduct() function from the Whitelist contract.
Then, a user can call the createOtoken() function from the OtokenFactory with the same assets and option type, and because the product is whitelisted, the requirement on line 70 will succeed.
However, although the product has been whitelisted, the collateral itself may not be approved. This is because the whitelistProduct() function does not check against the isWhitelistedCollateral() function if that collateral is allowed in the platform or not. Therefore, the first engagement with the collateral will appear on line 613 from Controller.sol, where the transaction will revert when the user wants to deposit some collateral in their vault.
Consider validating if the assets involved in a product have been already whitelisted before allowing the creation of oTokens.
Update: Fixed in PR#290 where the collateral asset is required to be whitelisted during the process of whitelisting the product.
[M-2] ERC20 compliant assets may not be used
A new oToken can be created by calling the createOtoken() function from the OtokenFactory contract and passing the whitelisted assets, among other parameters.
During the initialization of the new oToken, the code calls the _getNameAndSymbol() function which retrieves the standardized symbol and name for that oToken.
Nevertheless, the same function calls every single asset involved in the oToken to get their symbol, but because the symbol() and name() function from the ERC20 standard are optionals, these external calls may fail if those are not implemented and the oToken will not be created.
Similarly, the decimals() function is optional and ERC20 compliant assets may not include such function. Although in this scenario an oToken could be created using those assets, any action that would trigger the _verifyFinalState() function from the Controller contract will revert either in line 90 or 101 from MarginCalculator.sol.
Consider being as general as possible and assuming that these functions may not be implemented in the whitelisted assets.
Update: The Opyn team explained their views regarding this issue in a follow-up discussion. They decided not to implement any fixes and, instead, they will rely on the admin process of whitelisting underlying ERC20 tokens that conform to the requirements of their system. In the future, if the need arises, they intend to upgrade the system to support a broader range of ERC20 tokens.
[M-3] Actions are undefined at the exact time of oToken expiry
The oToken contract defines an expiration time in which certain operations will no longer be active due to the expiration of the option. The option style used for the Protocol is the european option in which the exercise of the option comes at maturity.
Some actions that can be performed before the expiration time are depositing long oTokens or minting new oTokens; while after the expiration time it can be redeemed or settled a vault.
Nevertheless, at the exact time of expiry, none of the time-based actions will succeed because that moment is not included in either of those 2 conditions and the transaction will revert.
Because it is based on a European option, actions that happen after maturity should be able to be called at the exact time of expiry. Consider including and defining the expiration time to either of the 2 conditions so transactions will not revert.
Update: Fixed in PR#291.
[M-4] Use of transfer might render ETH impossible to withdraw
When withdrawing ETH deposits, the [PayableProxyController contract](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/external/proxies/PayableProxyController.sol#L19) uses Solidity’s transfer() function. This has some notable shortcomings when the withdrawer is a smart contract, which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:
The withdrawer smart contract does not implement a payable fallback function.
The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.
To prevent unexpected behavior and potential loss of funds, consider explicitly warning end-users about the mentioned shortcomings to raise awareness before they deposit Ether into the protocol. Additionally, note that the sendValue() function available in OpenZeppelin Contract’s Address library can be used to transfer the withdrawn Ether without being limited to 2300 gas units. Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the “Check-effects-interactions” pattern and using OpenZeppelin Contract’s [ReentrancyGuard contract](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v2.5.1/contracts/utils/ReentrancyGuard.sol). For further reference on why using Solidity’s transfer() is no longer recommended, refer to these articles:
Update: Fixed in PR#305.
[M-5] User can force the Controller contract to perform an undesired external call
The [Controller contract](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/Controller.sol#L27) is the users’ front gate. With it, they can interact with the protocol to either open a new vault, deposit collateral or redeem their oTokens.
Besides those actions, the contract allows the execution of a more general transaction to be used as a flash loan in other projects by formatting the call with the [CallArgs struct format](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/libs/Actions.sol#L137).
By doing this, if the user submits that action in the operate() function, the call would jump into the _call() function to then perform an external call to the callFunction() payable function in the [callee address](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/libs/Actions.sol#L141).
However, if the callee address is not a [CalleeInterface based contract](https://github.com/opynfinance/GammaProtocol/blob/d151621b33134789b29dc78eb89dad2b557b25b9/contracts/interfaces/CalleeInterface.sol#L9) but it has either a fallback or payable fallback function in it, and it is not restricted the whitelisted addresses, the call coming from the Controller contract will end up executing any code under the fallback function on behalf of the Controller contract’s address. This other address could be either an asset that may be part of the whitelisted assets in the protocol or a future contract of the project that allows the execution of sensitive actions by the Controller contract.
Consider preventing an external call on behalf of the Controller contract when the destination address is not a CalleeInterface type of contract and it is not a whitelisted address.
Last updated
