https://www.figma.com/file/eQhRGVDQcoRTd5455V8FvA/Integration-Tests?node-id=0%3A1
Refer to Test Exercise + Claim Collateral

OptionsContract.transferUnderlying(address,uint256) ignores return value by underlying.transfer(_addr,_amt)

Ensure that all the return values of the function calls are used.

Should check if option collateral is an ERC20 and not ETH when calling addERC20Collateral()

OptionsContract.isSafe(uint256,uint256) performs a multiplication on the result of a division:
-rightSideVal = (collateralAmt.mul(collateralToEthPrice)).div(strikeToEthPrice)
-stillSafe = leftSideVal <= rightSideVal.mul(10 ** exp)

Consider ordering multiplication prior division.

https://github.com/aparnakr/OptionsProtocol/blob/9363067b23278cd6d919cc0dad8ae0d4b489491f/contracts/OptionsFactory.sol#L48

There seems to be an inconsistency with the comment here and wanted to make sure that it's not an issue in other parts of the code (which I haven't reviewed yet) that calculate the exercise window.

If _windowSize is a UNIX time, then does it mean that the the window is actually: FROM [_windowSize] to [expiry]?

More likely, _windowSize is number of seconds from expiry...

Report by Slither:

INFO:Detectors:
OptionsContract.setDetails(string,string) (OptionsContract.sol#275-286) contains a tautology or contradiction:
	- require(bool,string)(decimals >= 0,1 oToken cannot protect less than the smallest unit of the asset) (OptionsContract.sol#282-285)

Reference

function setDetails(string calldata _name, string calldata _symbol)
        external
        onlyOwner
    {
        name = _name;
        symbol = _symbol;
        decimals = uint8(-1 * oTokenExchangeRate.exponent);
        require(
            decimals >= 0,
            "1 oToken cannot protect less than the smallest unit of the asset"
        );
    }

There's no need to check decimals >= 0 for an uint.

OptionsContract.calculateOTokens(uint256,OptionsContract.Number) performs a multiplication on the result of a division:
-numeratorVal = (collateralAmt.mul(collateralToEthPrice)).div(strikeToEthPrice)
-numOptions = numeratorVal.mul(10 ** exp).div(denomVal)

Consider ordering multiplication prior division.

solidity coverage and coveralls

Reentrancy in oToken.addERC20CollateralOption(uint256,uint256,address) (oToken.sol#160-167):
External calls:
- addERC20Collateral(msg.sender,amtCollateral) (oToken.sol#165)
- require(bool,string)(collateral.transferFrom(msg.sender,address(this),amt),Could not transfer in collateral tokens) (OptionsContract.sol#362-365)
State variables written after the call(s):
- issueOTokens(amtToCreate,receiver) (oToken.sol#166)
- _balances[account] = _balances[account].add(amount) (@openzeppelin/contracts/token/ERC20/ERC20.sol#176)
- issueOTokens(amtToCreate,receiver) (oToken.sol#166)
- _totalSupply = _totalSupply.add(amount) (@openzeppelin/contracts/token/ERC20/ERC20.sol#175)

Apply the check-effects-interactions pattern.

Reentrancy in OptionsContract.addERC20Collateral(address,uint256):
External calls:
- require(bool,string)(collateral.transferFrom(msg.sender,address(this),amt),Could not transfer in collateral tokens)
State variables written after the call(s):
- _addCollateral(vaultOwner,amt)
- vault.collateral = vault.collateral.add(amt)

Apply the check-effects-interactions pattern.

OptionsContract.hasVault(address).owner (OptionsContract.sol#301) shadows:
- Ownable.owner() (@openzeppelin/contracts/ownership/Ownable.sol#29-31) (function)

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

Refer to https://www.figma.com/file/eQhRGVDQcoRTd5455V8FvA/Integration-Tests?node-id=0%3A1 Scenario:Add / Remove / Liquidate / Burn

I think the ABIs files in the ABIs folder are not up-to-date.

OptionsFactory.constructor(OptionsExchange,address)._oracleAddress lacks a zero-check on :
- oracleAddress = _oracleAddress

Check that the address is not zero.

Set up an admin for the options contract. The admin should be able to change the following collateralizationRatio, liquidationFactor, liquidationFee, liquidationIncentive, transactionFee

https://github.com/aparnakr/OptionsProtocol/blob/9363067b23278cd6d919cc0dad8ae0d4b489491f/contracts/OptionsFactory.sol#L124

Check if the same asset is being added. Something like:

require( tokens[_asset] != ERC20(_addr), ...)

Whenever the liquidate function gets called, the insurance smart contract should get liquidationFee amount of fees. The max amount of collateral that the liquidator can take out is liquidationFactor * repo.collateral. The amount of collateral taken out is calculated base on how many _oTokens are brought as: _oTokens * strikePrice * (LiquidationIncentive + LiquidationFee)

This is a low priority issue but something you may want to fix at some point.

There should be consistency when looking for the existence of a supported asset and returning the value for that supported asset.

The code below does an explicit check to see if "ETH" is a supported asset:

https://github.com/aparnakr/OptionsProtocol/blob/9363067b23278cd6d919cc0dad8ae0d4b489491f/contracts/OptionsFactory.sol#L154

However in the below code, it's assume that the lack of existence in the token[] is the value for the "ETH" token.

https://github.com/aparnakr/OptionsProtocol/blob/9363067b23278cd6d919cc0dad8ae0d4b489491f/contracts/OptionsFactory.sol#L75

This would be an issue in situations as follows:

1. Someone accidentally calls addAsset("ETH", 123) - with a non-zero _addr).
2. Solidity implementation change where token["ETH"] doesn't return 0. Perhap they change it to -1. This is unlikely to happen.

In both of the above situations, the rest of the system will break down since isETH() is expected to be ERC20(0).

Each time exercise is called, the exerciser gets strikePrice * _oTokens amount of collateral. The protocol gets transactionFee percent of the repo's collateral. Update the amountExercised = amount paid out the exerciser + the fee taken by the protocol + amountExercised.

CircleCI or Travis-CI

Fix the following issues list.

Audit Report by Open Zeppelin
Link: https://blog.openzeppelin.com/private-report-jan-6/
Password: ask for this to the owner

To Implement / Fix:

• L03 - WONT FIX
• L05
• L08
• N02
• N03
• N04 - Aparna
• N07
• N08
• N09
• N12
• N13
• N15
• N18
• L02
• L09
• L10

➜ OptionsProtocol git:(dev) node -v
v14.10.1
➜ OptionsProtocol git:(dev) npm -v
6.14.8
➜ OptionsProtocol git:(dev)
➜ OptionsProtocol git:(dev) npm i

[email protected] preinstall /Users/phil/DeFi/opyn/OptionsProtocol/node_modules/scrypt
node node-scrypt-preinstall.js

[email protected] install /Users/phil/DeFi/opyn/OptionsProtocol/node_modules/scrypt
node-gyp rebuild

SOLINK_MODULE(target) Release/copied_files.node
CC(target) Release/obj.target/scrypt_wrapper/src/util/memlimit.o
CC(target) Release/obj.target/scrypt_wrapper/src/scryptwrapper/keyderivation.o
CC(target) Release/obj.target/scrypt_wrapper/src/scryptwrapper/pickparams.o
CC(target) Release/obj.target/scrypt_wrapper/src/scryptwrapper/hash.o
LIBTOOL-STATIC Release/scrypt_wrapper.a
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/lib/crypto/crypto_scrypt.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/lib/crypto/crypto_scrypt_smix.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/libcperciva/util/warnp.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/libcperciva/alg/sha256.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/libcperciva/util/insecure_memzero.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/lib/scryptenc/scryptenc_cpuperf.o
LIBTOOL-STATIC Release/scrypt_lib.a
CXX(target) Release/obj.target/scrypt/src/node-boilerplate/scrypt_common.o
../src/node-boilerplate/scrypt_common.cc:98:12: warning: address of stack memory associated with local
variable 'scrypt_err_description' returned [-Wreturn-stack-address]
return scrypt_err_description.c_str();
^~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
CXX(target) Release/obj.target/scrypt/src/node-boilerplate/scrypt_params_async.o
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
In file included from ../src/node-boilerplate/inc/scrypt_async.h:28:
../src/node-boilerplate/inc/scrypt_common.h:39:14: error: no matching member function for call to 'Get'
N(obj->Get(Nan::New("N").ToLocalChecked())->Uint32Value()),
~~~~~^~~
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3717:43: note: candidate function not
viable: requires 2 arguments, but 1 was provided
V8_WARN_UNUSED_RESULT MaybeLocal Get(Local context,
^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3720:43: note: candidate function not
viable: requires 2 arguments, but 1 was provided
V8_WARN_UNUSED_RESULT MaybeLocal Get(Local context,
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
In file included from ../src/node-boilerplate/inc/scrypt_async.h:28:
../src/node-boilerplate/inc/scrypt_common.h:40:14: error: no matching member function for call to 'Get'
r(obj->Get(Nan::New("r").ToLocalChecked())->Uint32Value()),
~~~~~^~~
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3717:43: note: candidate function not
viable: requires 2 arguments, but 1 was provided
V8_WARN_UNUSED_RESULT MaybeLocal Get(Local context,
^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3720:43: note: candidate function not
viable: requires 2 arguments, but 1 was provided
V8_WARN_UNUSED_RESULT MaybeLocal Get(Local context,
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
In file included from ../src/node-boilerplate/inc/scrypt_async.h:28:
../src/node-boilerplate/inc/scrypt_common.h:41:14: error: no matching member function for call to 'Get'
p(obj->Get(Nan::New("p").ToLocalChecked())->Uint32Value()) {}
~~~~~^~~
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3717:43: note: candidate function not
viable: requires 2 arguments, but 1 was provided
V8_WARN_UNUSED_RESULT MaybeLocal Get(Local context,
^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3720:43: note: candidate function not
viable: requires 2 arguments, but 1 was provided
V8_WARN_UNUSED_RESULT MaybeLocal Get(Local context,
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
../src/node-boilerplate/inc/scrypt_async.h:53:17: warning: 'Call' is deprecated
[-Wdeprecated-declarations]
callback->Call(1, argv);
^
../../nan/nan.h:1741:3: note: 'Call' has been explicitly marked deprecated here
NAN_DEPRECATED inline v8::Localv8::Value
^
../../nan/nan.h:106:40: note: expanded from macro 'NAN_DEPRECATED'

define NAN_DEPRECATED attribute((deprecated))

                                   ^

In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
../src/node-boilerplate/inc/scrypt_params_async.h:35:36: error: too few arguments to function call,
single argument 'context' was not specified
maxtime(info[0]->NumberValue()),
~~~~~~~~~~~~~~~~~~~~ ^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:2861:3: note: 'NumberValue' declared here
V8_WARN_UNUSED_RESULT Maybe NumberValue(Local context) const;
^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8config.h:431:31: note: expanded from macro
'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT attribute((warn_unused_result))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
../src/node-boilerplate/inc/scrypt_params_async.h:36:39: error: too few arguments to function call,
single argument 'context' was not specified
maxmemfrac(info[1]->NumberValue()),
~~~~~~~~~~~~~~~~~~~~ ^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:2861:3: note: 'NumberValue' declared here
V8_WARN_UNUSED_RESULT Maybe NumberValue(Local context) const;
^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8config.h:431:31: note: expanded from macro
'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT attribute((warn_unused_result))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
../src/node-boilerplate/inc/scrypt_params_async.h:37:36: error: too few arguments to function call,
single argument 'context' was not specified
maxmem(info[2]->IntegerValue()),
~~~~~~~~~~~~~~~~~~~~~ ^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:2863:3: note: 'IntegerValue' declared here
V8_WARN_UNUSED_RESULT Maybe<int64_t> IntegerValue(
^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8config.h:431:31: note: expanded from macro
'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT attribute((warn_unused_result))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
../src/node-boilerplate/inc/scrypt_params_async.h:38:39: error: too few arguments to function call,
single argument 'context' was not specified
osfreemem(info[3]->IntegerValue())
~~~~~~~~~~~~~~~~~~~~~ ^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:2863:3: note: 'IntegerValue' declared here
V8_WARN_UNUSED_RESULT Maybe<int64_t> IntegerValue(
^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8config.h:431:31: note: expanded from macro
'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT attribute((warn_unused_result))
^
../src/node-boilerplate/scrypt_params_async.cc:23:8: error: no matching member function for call to
'Set'
obj->Set(Nan::New("N").ToLocalChecked(), Nan::New(logN));

/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3670:37: note: candidate function not
    viable: requires 3 arguments, but 2 were provided
V8_WARN_UNUSED_RESULT Maybe<bool> Set(Local<Context> context,
                                  ^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3673:37: note: candidate function not
    viable: requires 3 arguments, but 2 were provided
V8_WARN_UNUSED_RESULT Maybe<bool> Set(Local<Context> context, uint32_t index,
                                  ^
../src/node-boilerplate/scrypt_params_async.cc:24:8: error: no matching member function for call to
    'Set'
obj->Set(Nan::New("r").ToLocalChecked(), Nan::New<Integer>(r));
~~~~~^~~
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3670:37: note: candidate function not
    viable: requires 3 arguments, but 2 were provided
V8_WARN_UNUSED_RESULT Maybe<bool> Set(Local<Context> context,
                                  ^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3673:37: note: candidate function not
    viable: requires 3 arguments, but 2 were provided
V8_WARN_UNUSED_RESULT Maybe<bool> Set(Local<Context> context, uint32_t index,
                                  ^
../src/node-boilerplate/scrypt_params_async.cc:25:8: error: no matching member function for call to
    'Set'
obj->Set(Nan::New("p").ToLocalChecked(), Nan::New<Integer>(p));
~~~~~^~~
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3670:37: note: candidate function not
    viable: requires 3 arguments, but 2 were provided
V8_WARN_UNUSED_RESULT Maybe<bool> Set(Local<Context> context,
                                  ^
/Users/phil/Library/Caches/node-gyp/14.10.1/include/node/v8.h:3673:37: note: candidate function not
    viable: requires 3 arguments, but 2 were provided
V8_WARN_UNUSED_RESULT Maybe<bool> Set(Local<Context> context, uint32_t index,
                                  ^
../src/node-boilerplate/scrypt_params_async.cc:32:13: warning: 'Call' is deprecated
    [-Wdeprecated-declarations]
callback->Call(2, argv);
          ^
../../nan/nan.h:1741:3: note: 'Call' has been explicitly marked deprecated here
NAN_DEPRECATED inline v8::Local<v8::Value>
^
../../nan/nan.h:106:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                     ^
2 warnings and 10 errors generated.
make: *** [Release/obj.target/scrypt/src/node-boilerplate/scrypt_params_async.o] Error 1
gyp ERR! build error
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/Users/phil/.nvm/versions/node/v14.10.1/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:194:23)
gyp ERR! stack     at ChildProcess.emit (events.js:314:20)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12)
gyp ERR! System Darwin 20.1.0
gyp ERR! command "/Users/phil/.nvm/versions/node/v14.10.1/bin/node" "/Users/phil/.nvm/versions/node/v14.10.1/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /Users/phil/DeFi/opyn/OptionsProtocol/node_modules/scrypt
gyp ERR! node -v v14.10.1
gyp ERR! node-gyp -v v5.1.0
gyp ERR! not ok
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules/scrypt):
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] install: `node-gyp rebuild`
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: Exit status 1

up to date in 21.672s

40 packages are looking for funding
run `npm fund` for details

Reentrancy in oToken.addAndSellERC20CollateralOption(uint256,uint256,address) (oToken.sol#197-211):
External calls:
- addERC20Collateral(msg.sender,amtCollateral) (oToken.sol#202)
- require(bool,string)(collateral.transferFrom(msg.sender,address(this),amt),Could not transfer in collateral tokens) (OptionsContract.sol#362-365)
State variables written after the call(s):
- issueOTokens(amtToCreate,address(this)) (oToken.sol#203)
- _balances[account] = _balances[account].add(amount) (@openzeppelin/contracts/token/ERC20/ERC20.sol#176)
- issueOTokens(amtToCreate,address(this)) (oToken.sol#203)
- _totalSupply = _totalSupply.add(amount) (@openzeppelin/contracts/token/ERC20/ERC20.sol#175)

Apply the check-effects-interactions pattern.