opynfinance / gammaprotocol Goto Github PK

It's too complicated now

Modify getVault() name.

• Update testing files to be consistent in the BigNumber, toNumber(), toString() usage.
• Scale up all expected results variables or scale down tokens amount.

• Refactor AddressBook setup
• Refactor createTokenAmount
• change numbers to BigNumber

Figure out a way to check if the create2 address is correct.

Currently the Oracle module store one price for each batch at a specific time, if we have multiple ETH-USDC put options with different strike prices, we will only need to push the underlying asset price once to settle them all.
In case we have ETH put options with different collateral(USDC and DAI) we will need to push the price twice at expiry time as the batch for those two options is different. Also as the Oracle store an asset price, I think it will be better to map price directly by asset rather than by batch.

Add a terminator address instead of Controller's owner address, that can trigger the emergency shutdown.

https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#utilities

After some research I found that the recommended way to deal with ERC20 transfer and transfer from is to use the library provided by OZ.

Need to test the following:

1. Chainlink Prices work as expected
2. cToken exchange rate + prices work as expected
3. We can trade on the trading venue that we decided

https://crytic.io/opynfinance/GammaProtocol/54/security_checks

Crytic is complaining about our delete syntax on the vault struct. I ignore it for now because it seems fine and we already had test coverage the delete action. But probably need a deeper review before closing this.

MarginAccount._clearVault(MarginAccount.Vault) uses a variable after it was deleted
- Delete: delete _vault.longOtokens
- Use: delete _vault.collateralAssets

Make sure variables that are deleted are not used later.

• Move uintToInt() and intToUint() to another library
• Use struct to represent fixed point variables

[Issue] the getExcessCollateral returns a 1e18 number, instead of a scaled value for a long spread which settles ITM, without collateral deposited.
Scenario:
Long Option: $300 Strike Put
Short Option: $200 Strike Put
Collateral: USDC
Long amount: 10
Short amount: 10
Collateral amount: 0
On settle vault will fail because in margin calculator, in the get excessCollateral function, hasCollateral will be false, so it won't convert it into token amount.

Otoken._getOptionType(bool).symbol shadows:
- ERC20Initializable.symbol() (function)

Rename the local variable so as not to mistakenly overshadow any state variable/function/modifier/event definitions.

Update the function createScaledNumber() and createTokenAmount() to return a BigNumber instead of a string, will be more scalable for different use cases.

All of our european options are autoexercised. Should we rename this action as it might be confusing? No one needs to actually exercise anything. They just have to withdraw proceeds.

Some ideas:
WithdrawLongProceeds
WithdrawProceeds
WithdrawOtokenProceeds
RedeemLongProceeds
RedeemLong
Redeem
RedeemOtoken
WithdrawTheMoneys

Clean up all the npm scripts, make sure we need them all

Add integration tests for all blacklist functionalities in the Whitelist module.

on settleVault (for a long spread), it results in a revert transaction because there is no $ being transferred from the margin pool if the option is OTM. This makes it hard to clear the vault and the vault becomes unusable after that.

Update all the comments with proper descriptions for collateral, underlying and strike.

1. Use otoken series instead of otoken to represent a specific strike-underlying-collateral-strikePrice-expiry combination

Naming:

variables:
pauser
terminator
systemPaused
systemShutdown

functions:
TerminatorUpdated
PauserUpdated
SystemPaused
EmergencyShutdown

systemShutdown or emergency shutdown is done by terminator
systemPaused is done by pauser

I think we should switch names or change them. I think we should pick emergency shutdown vs system shutdown.

To me emergency shutdown implies that users will be able to unwind positions/remove collateral (kind of like maker, but at some future point in the future).
System paused implies to me that things are temporarily frozen - which I think applies to the entire protocol.

Any thoughts?

Otoken._getNameAndSymbol().name shadows:
- ERC20Initializable.name() (function)

Rename the local variable so as not to mistakenly overshadow any state variable/function/modifier/event definitions.

The current factory will only allow creation of option that expires at 08:00AM UTC.

Should we somehow make it possible to bypass this in case we need it? (Whitelist / admin?)

This will prevent launching an ETH-USDC-USDC call option with our first version.

This test fails sometimes

Otoken.init(address,address,address,uint256,uint256,bool).symbol shadows:
- ERC20Initializable.symbol() (function)

Rename the local variable so as not to mistakenly overshadow any state variable/function/modifier/event definitions.

In the current implementation, we ignore the returned value of transfer and transferFrom. In the ERC20 standard, a failed transfer should return false instead of revert.

May need to do some more research on if there're any tokens that won't return true, and look into other protocols on how they handle it.

In the current spec, there exist multiple checks that require having collateral address different than address zero.
We should decide wether to use WETH token or what specific address for ETH collateral.

Change const expiry = 1753776000 // 07/29/2025 @ 8:00am (UTC) to a dynamic value.

Create truffle script for easier dev experience

• Whitelist a long Otoken
• Whitelist a collateral
• Add a pricer
• Farm asset from pool
• Set terminator address
• Set disputer address
  ...

In file test/integration-tests/longCallSpreadExpireItm.test.ts.

get result of getExcessCollateral is wrong

      // Todo: Fix following rounding problem
      assert.equal(
        new BigNumber(vaultStateBeforeSettlement[0]).plus(1).toString(), // -4999999999999999999
        createScaledUint256(collateralPayout, (await weth.decimals()).toNumber()), //+5000000000000000000
      )

We've been ignoring the controller from the coverage report, and it turns out there're few lines of coverage missing.

https://coveralls.io/builds/33380934/source?filename=contracts/Controller.sol

Right now the intToUint() function is confusing because it will cast -5 to 5, @aparnakr suggested adding a different abs() method to deal with that, and let intToUint() revert when the input value is negative

Otoken._getMonth(uint256).symbol shadows:
- ERC20Initializable.symbol() (function)

Rename the local variable so as not to mistakenly overshadow any state variable/function/modifier/event definitions.