Byte Masons - Reliquary security record

The permission management system is based on a hierarchical permission set. This set is checked on each required function using a function call to `_atLeastRole()`. However, we found multiple implementations of this function with the same code. #### Contract: `ReaperBaseStrategyV4.sol` ```solidity=195 function _atLeastRole(bytes32 role) internal view { ... } ``` #### Contract: `ReaperVaultV2.sol` ```solidity=658 function _atLeastRole(bytes32 role) internal view { ... } ```

The comment states `This may only be called by governance or the strategist.`, however, the function `setEmergencyExit()` only allows `GUARDIAN` and roles with the higher privilege to call it. The `strategist` won't be able to activate the emergency exit.

The function `withdraw()` and the function `_withdraw()` have the parameters in different orders which may lead to unwanted withdraw operations. The previous functions are defined as: ```solidity=202 function withdraw( uint256 assets, address receiver, address owner ) ``` ```solidity=356 function _withdraw( uint256 _shares, address _owner, address _receiver ) ```

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.

During contract initialization call to `__ReaperBaseStrategy_init()` the following **multisig** account are given some permissions: ```solidity=75 _grantRole(DEFAULT_ADMIN_ROLE, _multisigRoles[0]); _grantRole(ADMIN, _multisigRoles[1]); _grantRole(GUARDIAN, _multisigRoles[2]); ``` However, the call to `__ReaperBaseStrategy_init()` may be reverted if the length of `_multisigRoles` is less than 3.

During the contract upgrade process, a function call is done to `clearUpgradeCooldown()` to flag the current upgrade process as finished. This function will set a future upgrade proposal time as follows: ```solidity=174 upgradeProposalTime = block.timestamp + (ONE_YEAR * 100); ``` In this way, the developers make sure that the next upgrade proposal time is set for the next 100 years in the future. However, this could be improved using a constant factor.

The auditing team would like to confirm with the team that the logic operations made over `availableCapital()` and `_liquidateAllPositions()` are correctly designed including edge cases in which: * `availableCapital()` returns zero; * `availableCapital()` returns negative values, and hence calling `_harvestCore(-availableCapital)` when no emergency exit is set; * On emergency exit set, verify that `amountFreed < debt`and `amountFreed > debt` work as expected when `debt` might be zero; * On emergency exit set, verify that when `amountFreed == debt` the `roi` value becomes zero and the `repayment` is set to `debt` for reporting.

The value `2**256 - 1` defined in function `maxDeposit()` can be declared as constant.

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.