Ethos Reserve contest - 0xsomeone's results

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L192 https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol#L157

Vulnerability details

Impact

The Ethos Vault ecosystem contains an access control inconsistency whereby a lesser role can have a significant impact by bypassing the role requirement via a different contract.

The ReaperBaseStrategyv4::setEmergencyExit function is meant to set a strategy to "emergency" mode by setting its allocation to 0 and explicitly setting its emergency flag to true. This function applies the GUARDIAN role AC requirement which is higher than the STRATEGIST role as defined in ReaperBaseStrategyv4::_cascadingAccessRoles.

The ReaperVaultV2::updateStrategyAllocBPS function is meant to set a strategy's allocation, enabling it to be set to 0. This function applies the STRATEGIST role AC requirement which is lower than the GUARDIAN role as defined in ReaperVaultV2::_cascadingAccessRoles. This effectively permits a STRATEGIST to bypass the GUARDIAN requirement and enforce an effectual emergency mode on a strategy by setting its allocation to 0.

Proof of Concept

The following code illustrates the issue in action:

contract AccessControlFlaw {
    bytes32 public constant STRATEGIST = keccak256("STRATEGIST");

    function demonstrate(IStrategy strategy, IVault vault) external {
        // Pre-Requisites
        require(vault.hasRole(address(this), STRATEGIST);
        require(strategy.hasRole(address(this), STRATEGIST);
        
        try strategy.setEmergencyExit() {
            revert();
        } catch (bytes memory e) {
            // The `STRATEGIST` role is unable to invoke `ReaperBaseStrategyv4::setEmergencyExit` as expected
        }
        
        try vault.updateStrategyAllocBPS(address(strategy), 0) {
            revert("ISSUE: The `STRATEGIST` role was able to set the allocation of a strategy to 0, thereby simulating its emergency exit scenario");
        } catch (bytes memory e) {
        }
    }
}

Tools Used

Manual review of the codebase.

Recommended Mitigation Steps

The adjustment required for the codebase depends on the business requirements of the Reaper ecosystem.

In our opinion, the current access control structure is meant to be retained in the code and the ReaperVaultV2::updateStrategyAllocBPS function should enforce a minimum BPS that each strategy must possess.

This will prevent a strategy from being removed unless a higher access role is possessed, similar to how strategy additions are guarded by the highest role possible.

Ethos Reserve Q&A Report

This submission serves as a Q&A report of the Ethos Reserve codebase. The findings that were identified with a low / non-critical security impact are as follows:

CommunityIssuance.sol

QIE-01L: Inexistent Evaluation of Reward Per Second (Affected Lines: L112)

An abnormally large distributionPeroid coupled with a low amount of funds in a CommunityIssuance::fund call will result in no rewards being distributed per second, causing fund loss that at most equates to distributionPeriod in the token's denomination.

We advise any unaccounted-for reward to not be transferred from the msg.sender to the contract by calculating the OathToken to transfer as rewardPerSecond.mul(distributionPeriod).

QIE-02L: Inexistent Sanitization of Distribution Period (Affected Lines: L120-L122)

The distribution period of the contract is not sanitized during a CommunityIssuance::updateDistributionPeriod call.

We advise a non-zero validation to be performed as otherwise the contract's CommunityIssuance::fund function will be inoperable until the misconfiguration is corrected.

CollateralConfig.sol

CCG-01L: Inexistent Validation of System Conditions (Affected Lines: L66-L70)

The CCR configured for a particular market should at all times be greater than the MCR as otherwise, the market will never achieve its critical-state, preventing the system's failsafe mechanisms from kicking in.

We advise the _CCRs[i] value to be mandated as greater-than-or-equal-to the _MCRs[i] value of a particular collateral. Depending on how this finding is viewed, it can be considered medium-risk as a misconfigured asset will never trigger the failsafe mechanisms of the protocol and thus never recover from a critical state.

ReaperBaseStrategyv4.sol

RBS-01L: Incorrect Usage of Deprecated Method (Affected Lines: L74)

The referenced line invokes SafeERC20::safeApprove which can cause upgrades to fail as the ReaperBaseStrategyv4 contract represents an upgradeable contract.

If the contract's logic is adjusted and attempted to be upgraded, a second SafeERC20::safeApprove invocation that will be performed during the contract's re-initialization will fail due to the custom and deprecated logic present in the function.

We advise a direct approve instruction to be utilized instead, potentially by evaluating the yielded bool opportunistically similarly to how SafeERC20 achieves this.

RBS-02L: Unsafe Value Negations (Affected Lines: L114, L121, L128)

The referenced negation operations (-a) are performed unsafely as they do not apply any bound checks to the a value.

As all linked negations are meant to convert a value from a negative to a positive value, an edge case manifests whereby the minimum int256 (type(int256).min) will overflow when negated by one unit as the negative value ranges have one more member in signed integer types.

We advise the code to use a wrapper library that negates the value and applies a bound check to ensure that the negation will never result in an underflow.

ReaperStrategyGranarySupplyOnly.sol

RGO-01L: Unsafe Casting Operations (Affected Lines: L134, L141)

The referenced casting operations are performed unsafely, potentially resulting in an uncaught casting overflow to occur.

We advise safe wrapper libraries to be utilized, such as OpenZeppelin's SafeCast library. To note, Solidity's built-in safe arithmetic post-0.8 does not cover casting overflows.

RGO-02L: Unsafe Value Negation (Affected Lines: L136)

The referenced negation operations (-a) are performed unsafely as they do not apply any bound checks to the a value.

As all linked negations are meant to convert a value from a negative to a positive value, an edge case manifests whereby the minimum int256 (type(int256).min) will overflow when negated by one unit as the negative value ranges have one more member in signed integer types.

We advise the code to use a wrapper library that negates the value and applies a bound check to ensure that the negation will never result in an underflow.

ReaperVaultV2.sol

RVT-01L: Inconsistent Input Sanitization (Affected Lines: L133-L135)

The referenced lines of code utilize the _multisigRoles members without validating the array's length.

We advise sanitization to be imposed similarly to ReaperBaseStrategyv4::__ReaperBaseStrategy_init to prevent improper role initializations.

RVT-02L: Unsafe Casting Operations (Affected Lines: L228, L235, L248)

The referenced casting operations are performed unsafely, potentially resulting in an uncaught casting overflow to occur.

We advise safe wrapper libraries to be utilized, such as OpenZeppelin's SafeCast library. To note, Solidity's built-in safe arithmetic post-0.8 does not cover casting overflows.

RVT-03L: Inexistent Validation of Duplicate Entries (Affected Lines: L258-L271)

The ReaperVaultV2::setWithdrawalQueue function does not evaluate whether duplicate entries exist in the queue.

While the finding will have no impact as it will only result in a redundant increase in gas within ReaperVaultV2::_withdraw, we still advise duplicate entries to be prohibited.

RVT-04L: Inexistent Sanitization of Withdrawal Queue (Affected Lines: L258-L271)

The active withdrawal queue of the system should at all times represent the strategies from which funds can be withdrawn in the system.

We advise the length of the queue to be mandated as exactly equal to the number of active strategies, ensuring that an incorrect withdrawal queue cannot be specified.

RVT-05L: Unsafe Value Negations (Affected Lines: L500, L510)

The referenced negation operations (-a) are performed unsafely as they do not apply any bound checks to the a value.

As all linked negations are meant to convert a value from a negative to a positive value, an edge case manifests whereby the minimum int256 (type(int256).min) will overflow when negated by one unit as the negative value ranges have one more member in signed integer types.

We advise the code to use a wrapper library that negates the value and applies a bound check to ensure that the negation will never result in an underflow.

ReaperVaultERC4626.sol

RVE-01L: Misleading Function Naming (Affected Lines: L202-L210)

The ReaperVaultERC4626::withdraw function meant to conform to the EIP standard accepts the input uint256 argument denominated in assets, however, the ReaperVaultV2::withdraw functions accept the input uint256 argument denominated in shares.

We advise the codebase to become consistent, utilizing a different naming convention for ReaperVaultV2 (i.e. deposit).

ActivePool.sol

APL-01L: Unsafe Casting Operations (Affected Lines: L277)

The referenced casting operations are performed unsafely, potentially resulting in an uncaught casting overflow to occur.

We advise safe wrapper libraries to be utilized, such as OpenZeppelin's SafeCast library. To note, Solidity's built-in safe arithmetic post-0.8 does not cover casting overflows.

APL-02L: Unsafe Value Negations (Affected Lines: L282)

The referenced negation operations (-a) are performed unsafely as they do not apply any bound checks to the a value.

As all linked negations are meant to convert a value from a negative to a positive value, an edge case manifests whereby the minimum int256 (type(int256).min) will overflow when negated by one unit as the negative value ranges have one more member in signed integer types.

We advise the code to use a wrapper library that negates the value and applies a bound check to ensure that the negation will never result in an underflow.

APL-03L: Misleading Error Message (Affected Lines: L334)

The referenced error message of the require check within ActivePool::_requireCallerIsBOorTroveMorSP contains a misleading error message which does not list the redemptionHelper.

We advise this message to be corrected, representing the actual condition evaluated by the require check.

BorrowerOperations.sol

BOS-01L: Incorrect Relayed Borrower (Affected Lines: L365)

The BorrowerOperations::_adjustTrove function will relay an incorrect "borrower" to its internal _moveTokensAndCollateralfromAdjustment call when BorrowerOperations::moveCollateralGainToTrove is invoked by the stability pool.

While it is inconsequential in the current code (no LUSD is withdrawn / repaid and the transaction represents a _isCollIncrease), it is still advisable to relay the correct argument via a ternary (a ? b : c) conditional.

StabilityPool.sol

SPL-01L: Unconditional Loss Calculation (Affected Lines: L339, L384)

The referenced calculations meant to assess the LUSD loss incurred by the deposit are unconditionally executed, potentially causing an underflow if the position somehow increases.

We evaluated the codebase and the linked calculation can never underflow in the current implementation, however, the code should evaluate whether any loss exists prior to calculating it via a conditional, prohibiting an underflow from halting deposits / withdrawals.

LUSDToken.sol

LTN-01L: Permittence of Invalid Signatures (Affected Lines: L287-L288)

The ecrecover mechanism within LUSDToken::permit allows an invalid signature to result in the zero address incorrectly.

While an approval will still be prevented by how the LUSDToken::_approve function disallows the from address from being zero, we still advise invalid signatures to emit a proper error message rather than failing with an "approval from zero" error message.

LTN-02L: Bypass of Valid Recipient Check (Affected Lines: L188, L203)

The LUSDToken::returnFromPool as well as the LUSDToken::mint functions represent instances whereby a non-zero LUSD balance can be transmitted to a target recipient without the LUSDToken::_requireValidRecipient check.

We advise both instances to properly apply the check, preventing LUSD tokens from being minted to incorrect addresses. The present use of the codebase disallows this as no code path will cause the check to fail, however, the check should be present regardless.

Ethos Reserve Gas Optimization Report

This submission serves as a gas optimization report of the Ethos Reserve codebase. The ways the codebase can be optimized are as follows:

CommunityIssuance.sol

CIE-01G: Multiple Redundant Repetitive Storage Reads (Affected Lines: L86, L87, L88, L106, L107, L108, L112-L113, L116)

The referenced statements read the same value from the contract's storage multiple times in the same function context.

We advise them to instead cache the variable's value to a local in-memory variable instead, reducing the total warm SLOAD operations performed in the codebase.

CIE-02G: Redundant Usage of SafeMath (Affected Lines: L88, L107)

The referenced operations are guaranteed to be performed safely by the logical constraints (i.e. if clauses) that precede their execution.

We advise the usage of SafeMath in these instances to be omitted, optimizing their gas cost.

CIE-03G: Unconditional Emission of Event (Affected Lines: L94)

The referenced TotalOATHIssuedUpdated event is emitted regardless of whether the totalOATHIssued was actually adjusted.

We advise it to be relocated within the if clause, removing the potentially redundant SLOAD operation and ensuring the event is emitted optimally.

LQTYStaking.sol

LSG-01G: Multiple Redundant Repetitive Storage Reads (Affected Lines: L125, L160)

The referenced statements read the same value from the contract's storage multiple times in the same function context.

We advise them to instead cache the variable's value to a local in-memory variable instead, reducing the total warm SLOAD operations performed in the codebase.

LSG-02G: Redundant Usage of SafeMath (Affected Lines: L155, L181)

The referenced operations are guaranteed to be performed safely by the logical constraints (i.e. if clauses) that precede their execution.

We advise the usage of SafeMath in these instances to be omitted, optimizing their gas cost.

LSG-03G: Optimal Execution of Functions (Affected Lines: L181, L191)

The referenced if conditionals will cause their function to result in a no-op if they are not satisfied.

As such, we advise an else clause to be introduced to them that immediately stops the function's execution (i.e. return), optimizing their gas cost significantly.

LSG-04G: Inefficient Mapping Entry Usages (Affected Lines: L208-L209, L230, L234)

The referenced mapping variable usages do so using direct accessors (i.e. foo[bar][zoo]) instead of storing the mapping lookups to interim memory / storage variables.

We advise the lookups to be cached to local variables as described above, significantly optimizing their usage cost as each mapping lookup performs a keccak256 operation under-the-hood to assess the storage offset of the entry each time.

ReaperBaseStrategyv4.sol

RBS-01G: Inefficient Unchecked Increments (Affected Lines: L77)

The referenced lines perform unchecked increments of loop iterators, however, this is achieved via library calls which are delegatecall instructions at the EVM level.

We advise them to be performed directly, avoiding the unnecessary gas overhead of delegatecall instructions the current paradigm incurs.

RBS-02G: Inefficient Checked Arithmetic (Affected Lines: L121, L123)

The referenced statements perform Solidity's built-in safe arithmetics redundantly as their safety is guaranteed by the logical constraints (i.e. if clauses) that precede their execution.

We advise the usage of unchecked code blocks in these instances, optimizing their gas cost.

RBS-03G: Inefficient Calculation of Repayment (Affected Lines: L128)

Based on the calculations in ReaperBaseStrategyv4::harvest, the value of repayment will be equal to amountFreed if the roi is negative. As such, we advise the assignment to be made directly to amountFreed minimizing the calculations within the code.

Parting Note

More optimizations were identified, however, they were not able to be transcribed into legible text in time for the contest's duration. I am more than happy to provide them manually for the sponsor to review and consume within 24 hours of the contest's end.