Inverter A-1

Modules are intended to be upgradeable, and their implementation is determined by the InverterBeacon_v1, however there are extra considerations to be made, particularly to do with storage slot conflicts. These storage conflicts typically occur when an upstream contract has additional storage parameters added that weren’t present in a prior implementation, which takes a storage slot, and pushes the storage slot number used in downstream contracts down, which causes the downstream contracts to no longer point to their prior slot and the new storage variable may point to storage values already set. This can lead to many unintended consequences that can be quite severe.

Currently, there are several cases of upstream contracts using regular storage slots that could conflict with inheriting contract slots on upgrade. The FM_BC_Bancor_Redeeming_VirtualSupply_v1 funding manager contract has its own storage variables, while it inherits a chain of contracts: RedeemingBondingCurveBase_v1, BondingCurveBase_v1, and Module_v1.sol, each of which has its own storage variables.

It is likely that if there is an upgrade, some part of the upstream inheritance chain would require an additional storage variable, which would conflict with all storage downstream if added normally and would require a patchwork storage pattern to resolve.

Remediations to Consider

• Add storage gaps to the end of each contract that is intended to be upgradable, reserving slots for future use, or
• Implement the unstructured storage pattern to allow for the easy addition of storage variables for future module upgrades.

AUT_Roles_v1 is a base authorizer module used throughout a protocol to determine whether modules or users are authorized for particular functions. It has four major roles: the DEFAULT_ADMIN_ROLE, ORCHESTRATOR_OWNER_ROLE, ORCHESTRATOR_MANAGER_ROLE, and BURN_ADMIN_ROLE.

Each module in the protocol also has its own module role, and thus, its role admin is the DEFAULT_ADMIN_ROLE, which is owned by the ORCHESTRATOR_OWNER_ROLE, giving it control over setting members. The ORCHESTRATOR_OWNER_ROLE is the main owner of the protocol and has the highest authority, and is granted the admin over the DEFAULT_ADMIN_ROLE, giving it control of setting members of roles for all roles that do not have an explicitly set role admin, as well as directly being set as the admin of the ORCHESTRATOR_MANAGER_ROLE.

The BURN_ADMIN_ROLE is special as it is intended to be used to remove any admin control over a module role since it is not supposed to be controlled by anyone or have any members. A module role’s admin is burned when burnAdminFromModuleRole() is called by a module, making changes to roles immutable:

 /// @inheritdoc IAuthorizer_v1
  function burnAdminFromModuleRole(bytes32 role)
      external
      onlyModule(_msgSender())
  {
      bytes32 roleId = generateRoleId(_msgSender(), role);
      _setRoleAdmin(roleId, BURN_ADMIN_ROLE);
  }

Reference: AUT_Roles_v1.sol#L225-L232

However, in order for the BURN_ADMIN_ROLE to work as intended, it needs not to be controlled by any other role, which is done by setting its own admin to itself as is done in the constructor:

constructor() {
    // make the BURN_ADMIN_ROLE immutable
    _setRoleAdmin(BURN_ADMIN_ROLE, BURN_ADMIN_ROLE);
}

Reference: AUT_Roles_v1.sol#L86-L89

This should work for normal contracts, but this is intended to be a proxy contract where initialization is handled in a separate initializer function, anything done to storage in the constructor does not effect the storage of proxies that use it as an implementation contract. Since in its initializer function, there is no role admin set for the BURN_ADMIN_ROLE, its role admin will be the DEFAULT_ADMIN_ROLE, which its admin is set to be the ORCHESTRATOR_OWNER_ROLE, giving the ability for members of the ORCHESTRATOR_OWNER_ROLE to control module roles that are intended to be immutable, breaking the purpose of the BURN_ADMIN_ROLE.

Remediations to Consider

Set the BURN_ADMIN_ROLE’s admin to itself in the initializer rather than the constructor to ensure it is not controlled by any role and serves its intended purpose.

AUT_TokenGated_Roles_v1 is an authorizer module that allows roles to be set that are token gated, which means users have a given role if they have a sufficient amount of a set token, determined by a set threshold, which is checked in _hasTokenRole():

/// @notice Internal function that checks if an account qualifies for a token-gated role.
/// @param role The role to be checked.
/// @param who The account to be checked.
function _hasTokenRole(bytes32 role, address who)
    internal
    view
    returns (bool)
{
    uint numberOfAllowedTokens = getRoleMemberCount(role);

    for (uint i; i < numberOfAllowedTokens; ++i) {
        address tokenAddr = getRoleMember(role, i);
        bytes32 thresholdId = keccak256(abi.encodePacked(role, tokenAddr));
        uint tokenThreshold = thresholdMap[thresholdId];

        //Should work with both ERC20 and ERC721
        try TokenInterface(tokenAddr).balanceOf(who) returns (
            uint tokenBalance
        ) {
            if (tokenBalance >= tokenThreshold) {
                return true;
            }
        } catch {
            // If the call fails, we continue to the next token.
            // Emitting an event here would make this function (and the functions calling it) non-view.
            // note we already enforce Interface implementation when granting the role.
        }
    }

    return false;
}

Reference: AUT_TokenGated_Roles_v1.sol#L227-L257

However, for sufficiently liquid tokens, the balance can be manipulated within the context of a transaction to briefly exceed the required threshold via flash loans or similar interactions. This can allow users to gain access to token gated functions easily, but not actually own/hold the tokens.

Remediations to Consider

This may not be an issue if the tokens used are illiquid or non-transferrable, so users and integrators should be aware of this issue and take it into consideration when deciding eligible tokens to use to gate roles.

PP_Streaming_v1 is a payment processor module that performs linear payouts across time. It is meant to be used as an alternative to the PP_Simple module, which performs lump-sum payments. An appropriate payment processor module can be configured on the Orchestrator_v1 instance. The Orchestrator_v1:initiateSetPaymentProcessorWithTimelock() implementation performs a check if the provided module supports the IPaymentProcessor_v1 interface.

However, supportsInterface() in PP_Streaming_v1 is not correctly implemented and declares support only for IPP_Streaming_v1 and IModule_v1 interfaces (in Module_v1:supportsInterface()). IPP_Streaming_v1 does inherit from the IPaymentProcessor_v1, but that does not affect the return result of the supportsInterface() when it is executed. As a result, Orchestrator_v1:initiateSetPaymentProcessorWithTimelock() will revert when provided module for payment processing is PP_Streaming_v1.

// add to PP_StreamingV1Test
function testSupportsPPInterface() public {
    assertTrue(
        paymentProcessor.supportsInterface(
            type(IPaymentProcessor_v1).interfaceId
        )
    );
}

Remediations to consider

• Update PP_Streaming_v1:supportsInterface() implementation and instead of IPP_Streaming_v1 inheriting from the IPaymentProcessor_v1 make PP_Streaming_v1 inherit IPaymentProcessor_v1.

In PP_Simple_v1, the processPayments() is responsible for processing all pending orders. Orders are retrieved from the associated payment client and iterated through for final asset transfers.

If a single transfer in the whole batch fails and results in a revert when the external call is made, the whole batch is reverted, and processPayments() does not change the state.

token_.safeTransferFrom(address(client), recipient, amount);

Asset transfers may fail for various reasons, such as the recipient being blocked in USDC. A malicious user may set a blocked recipient for pending orders and cause order batches to fail repeatedly.

Currently, the PP_Simple_v1 module does not have a mechanism to ignore failed transfers, and the ERC20PaymentClientBase_v1 module does not feature the capability to remove unprocessable orders from the pending queue. As a result, one single bad transfer will block all payouts.

Remediations to consider

• In the PP_Simple_v1 implement a similar mechanism for failed transfer as in PP_Streaming_v1, where failed transfers become unclaimed and can be pulled by the corresponding asset recipient later.
• Also, consider having functionality for removing unprocessable orders from the pending orders in ERC20PaymentClientBase_v1 or its child contracts.

Throughout the protocol, modules query the Funding Managers token, interacting with it and/or storing balances denominated in this token. However, there is no guarantee that this token will not change since, in the Orchestrator, the FundingManager can be updated and does not ensure a constant token. A new funding manager could have a different value for its token, which would affect all other modules that depend on it, resulting in potential loss of funds or breaking the protocol's functionality.

Remediations to Consider

In Orchestrator_v1.sol, when changing the FundingManager, ensure the token is the same.

In LM_PC_KPIRewarder_v1.sol, the ASSERTER_ROLE calls postAssertion() with an assertion of data that supposedly relates to the KPI of a relevant target. This assertion is separated into 3 main data points: the asserted value, data, and dataId. Of these 3 parameters, only the data and dataId parameters are actually asserted to be true via the uma optimistic oracle, while the assertedValue is potentially unrelated. However, assertedValue is the only value that actually determines the outcome of the rewards. This can lead to cases where an asserter could provide data and dataId that claims an unrelated true fact, which will not be disputed, while the provided assertedValue can be anything, as it is unverified.

Remediations to Consider

• Consider including the assertedValue in the data being verified by the uma optimistic oracle.

In LM_PC_KPIRewarder_v1.sol, users can stake tokens to earn rewards based on KPI, which is verified by the uma optimistic oracle. However, when stake() is called, the funds are not immediately staked, but are added to a queue, so that they are ineligible for rewards on any pending KPI reward assertions. So when a new assertion is made by an address with the ASSERTER_ROLE by calling postAssertion(), all queued stake requests are processed:

...
// Staking Queue Management

for (uint i = 0; i < stakingQueue.length; i++) {
    address user = stakingQueue[i];
    _stake(user, stakingQueueAmounts[user]);
    totalQueuedFunds -= stakingQueueAmounts[user];
    stakingQueueAmounts[user] = 0;
}

delete stakingQueue; // reset the queue
...

Reference: LM_PC_KPIRewarder_v1.sol#L170-L179

However, to prevent the queue from becoming too long, there is a MAX_QUEUE_LENGTH when staking, preventing it from getting any larger.

/// @inheritdoc ILM_PC_Staking_v1
function stake(uint amount)
    external
    override
    nonReentrant
    validAmount(amount)
{
    if (stakingQueue.length >= MAX_QUEUE_LENGTH) {
        revert Module__LM_PC_KPIRewarder_v1__StakingQueueIsFull();
    }
    ...

Reference: LM_PC_KPIRewarder_v1.sol#L262-L271

The MAX_QUEUE_LENGTH is set to 50. Having a limit to the queue length can prevent a malicious user from staking on many addresses and expanding the queue to the point where it would exceed the gas limit to process during the postAssertion() call.

In adding this MAX_QUEUE_LENGTH, however, it does allow for a malicious user to block new stakers from staking if they stake using multiple accounts until the MAX_QUEUE_LENGTH is reached, preventing any other users from staking. This could be done to grief or to gain a monopoly on staking and earn all rewards themselves. In addition, it could be repeated each time a new assertion is made, which processes the queue.

Remediations to Consider

A couple of approaches could be made to resolve this issue, but in all MAX_QUEUE_LENGTH should be removed:

• redesign the contract not to require a queue, but rather have staked balances stored for the user but tied to the next KPICounter. Although this would take time and consideration, it would remove the issue entirely.
• Add a way for the ASSERTER_ROLE to process the queue in chunks, preventing the gas limit from being exceeded in a call to postAssertion(). Also, introduce a minimum stake amount to increase the capital requirement to grief. Note: this will not entirely remove the possibility of griefing from a user with enough capital, but the incentive to do so should be reduced enough to make it unlikely to occur.

In RedeemingBondingCurveBase_v1.sol’s _sellOrder(), it allows users to sell their issuance tokens for the collateral token. The amount of collateral token received for a given amount of issuance token is determined by the downstream contract implementation of the _redeemTokensFormulaWrapper().

The example current implementation, in FM_BC_Bancor_Redeeming_VirtualSupply_v1, uses virtual issuance and collateral supplies and a bonding curve to calculate the sell price, but there may be future implementations that use something simple like the total supply of issuance tokens compared to amount of collateral held to determine sell price. In cases where protocols use the total supply of issuance tokens in their pricing calculations it can lead to issues if there is also a protocol fee taken if the sellFee is set.

This is because before pricing is calculated via the implemented call to _redeemTokensFormulaWrapper(), sell fees are taken via minting issuance tokens to the protocol via _processProtocolFeeViaMinting(), which briefly increases the issuance tokens total supply:

 ...
 // Process the protocol fee
  _processProtocolFeeViaMinting(issuanceTreasury, protocolFeeAmount);
  // Calculate redeem amount based on upstream formula
  uint collateralRedeemAmount = _redeemTokensFormulaWrapper(netDeposit);

  totalCollateralTokenMovedOut = collateralRedeemAmount;

  // Burn issued token from user
  _burn(_msgSender(), _depositAmount);
  ...

Reference: RedeemingBondingCurveBase_v1.sol#L185-L193

In simple implementations that resemble vaults, if the value of the issuance token is proportional to the amount of collateral token over the issuance tokens total supply, ie each issuance token represents a share of the collateral held in the contract, then the protocol fee being minted beforehand would inflate the total supply and decrease the value of issuance tokens in this calculation, resulting in a lower sell price than expected and fewer collateral tokens received. This effect is magnified when the total supply is low, and if the last issuance tokens are attempted to be sold, the sellFee effectively doubles.

Remediations to Consider

Reorder the operations to call _redeemTokensFormulaWrapper() before minting the protocol fees to the issuanceTreasury via _processProtocolFeeViaMinting(); this ensures the supply of tokens that may be used to determine prices are correct and not inflated.

When the base Authorizer module, AUT_Roles_v1.sol, is initialized via __RoleAuthorizer_init(), an initialOwner parameter is expected, which sets the owner of the protocol that uses it as an authorizer, the ORCHESTRATOR_OWNER_ROLE.

...
if (initialOwner != address(0)) {
    _grantRole(ORCHESTRATOR_OWNER_ROLE, initialOwner);
} else {
        ///@audit: here the message sender becomes the owner, but that is likely the module factory
    _grantRole(ORCHESTRATOR_OWNER_ROLE, _msgSender());
}
...

Reference: AUT_Roles_v1.sol#L128-L132

Implementation allows the initialOwner to be set as the zero address, which in that case, sets the owner to be the msg.sender. However, in most cases, modules are intended to be deployed and initialized by the ModuleFactory_v1 contract via its createModule() function:

function createModule(
    IModule_v1.Metadata memory metadata,
    IOrchestrator_v1 orchestrator,
    bytes memory configData
) external returns (address) {
    // Note that the metadata's validity is not checked because the
    // module's `init()` function does it anyway.

    IInverterBeacon_v1 beacon;
    (beacon, /*id*/ ) = getBeaconAndId(metadata);

    if (address(beacon) == address(0)) {
        revert ModuleFactory__UnregisteredMetadata();
    }

    address implementation = address(new InverterBeaconProxy_v1(beacon));

    IModule_v1(implementation).init(orchestrator, metadata, configData);

    emit ModuleCreated(
        address(orchestrator),
        implementation,
        LibMetadata.identifier(metadata)
    );

    return implementation;
}

Reference: ModuleFactory_v1.sol#L115-L141

This becomes an issue because the ModuleFactory does not have the functionality to act as the owner and would brick the most important role of the protocol, requiring redeployment or an upgrade to resolve.

Remediations to Consider

Revert if the provided initialOwner is address(0) to prevent this invalid initialization from occurring.

In the LM_PC_KPIRewarder_v1, the implementation overrides assertionResolvedCallback from the parent OptimisticOracleIntegrator contract. Within parent implementation, assertionData[assertionId].resolved is set to true when it is successfully resolved, and assertionData[assertionId] is deleted when it is false.

However, within LM_PC_KPIRewarder_v1's implementation assertionData[assertionId] mapping is never updated. As a result, the corresponding entry will have a false value for the resolved field. In addition, OptimisticOracleIntegrator:getData() will return (false, 0) for already resolved assertions.

Remediations to Consider

• Update LM_PC_KPIRewarder_v1:assertionResolvedCallback() implementation to manage data structures related to the overridden parent functions properly.

Orchestrator_v1.sol requires that there always is an Authorizer, Funding Manager, and Payment Processor module set, with other nonessential modules able to be added or removed more freely. This is done by having specific functions to update these essential modules, like for the Authorizer:

/// @inheritdoc IOrchestrator_v1
function initiateSetAuthorizerWithTimelock(IAuthorizer_v1 authorizer_)
    external
    onlyOrchestratorOwner
{
    address authorizerContract = address(authorizer_);
    bytes4 moduleInterfaceId = type(IModule_v1).interfaceId;
    bytes4 authorizerInterfaceId = type(IAuthorizer_v1).interfaceId;
    if (
        ERC165Checker.supportsInterface(
            authorizerContract, moduleInterfaceId
        )
            && ERC165Checker.supportsInterface(
                authorizerContract, authorizerInterfaceId
            )
    ) {
        _initiateAddModuleWithTimelock(authorizerContract);
        _initiateRemoveModuleWithTimelock(address(authorizer));
    } else {
        revert Orchestrator__InvalidModuleType(address(authorizer_));
    }
}

/// @inheritdoc IOrchestrator_v1
function executeSetAuthorizer(IAuthorizer_v1 authorizer_)
    external
    onlyOrchestratorOwner
{
    _executeAddModule(address(authorizer_));
    _executeRemoveModule(address(authorizer));
    authorizer = authorizer_;
    emit AuthorizerUpdated(address(authorizer_));
}

/// @inheritdoc IOrchestrator_v1
function cancelAuthorizerUpdate(IAuthorizer_v1 authorizer_)
    external
    onlyOrchestratorOwner
{
    _cancelModuleUpdate(address(authorizer_));
}

Reference: Orchestrator_v1.sol#L208-L248

Removal of one of these modules using the normal functions is prevented by explicitly checking if the module is essential:

/// @inheritdoc IOrchestrator_v1
function initiateRemoveModuleWithTimelock(address module_) external {
    // Revert given module to be removed is equal to current authorizer
    if (module_ == address(authorizer)) {
        revert Orchestrator__InvalidRemovalOfAuthorizer();
    }
    // Revert given module to be removed is equal to current fundingManager
    if (module_ == address(fundingManager)) {
        revert Orchestrator__InvalidRemovalOfFundingManager();
    }
    // Revert given module to be removed is equal to current paymentProcessor
    if (module_ == address(paymentProcessor)) {
        revert Orchestrator__InvalidRemovalOfPaymentProcessor();
    }
    _initiateRemoveModuleWithTimelock(module_);
}

Reference: Orchestrator_v1.sol#L342-L357

However, if initiateSetAuthorizerWithTimelock() is called, it will call the same function _initiateRemoveModuleWithTimelock() without the above checks, which after a timelock period, the normal executeRemoveModule() can be called, removing the module without replacing it.

Remediations to Consider

In executeRemoveModule(), check if the module is an essential module, as is done in initiateRemoveModuleWithTimelock() to ensure that there is always one of each essential module.

In LM_PC_KPIRewarder_v1.sol, a queue of stakers is created to prevent new stakers from receiving the rewards of KPI assertions that are pending verification. Without this queue, users could frontrun the verification, stake to earn rewards then unstake in the next block. As mentioned in the contract description, the stakers are queued until the next assertion is resolved. However, this assumes that there is only ever 1 assertion pending a resolution. If an asserter were to call postAssertion() while there is another pending assertion, then the stakers queued would be added and eligible for rewards for both the new and prior pending assertion, which is not desired.

Remediations to Consider

Require that there is only ever 1 pending assertion to ensure fairness for stakers.

In OptimisticOracleIntegrator.sol and contracts that inherit it like LM_PC_KPIRewarder_v1.sol, the minimum bond is used to make an assertion:

       function assertDataFor(bytes32 dataId, bytes32 data, address asserter)
        public
        virtual
        onlyModuleRole(ASSERTER_ROLE)
        returns (bytes32 assertionId)
    {
        asserter = asserter == address(0) ? _msgSender() : asserter;
        uint bond = oo.getMinimumBond(address(defaultCurrency));
        ...

Reference: OptimisticOracleIntegrator.sol#L152-L159

This bond is returned if the assertion is not successfully disputed but is lost otherwise. As mentioned in the Uma docs: “If data received from the oracle could potentially move large amounts of value, you may want to set a higher bond to make it more costly for an attacker to attempt to steal funds through a false proposal.“ In the case where the ASSERTER_ROLE acts maliciously, they could post multiple proposals with invalid data, potentially with inflated KPI, with minimal loss on a dispute. Since the bond is low, and a disputer receives half the bond as an incentive, an assertion with a lower bond is less likely to be disputed, as it may not be worth disputing, so an invalid assertion may end up passing.

Remediations to Consider

Consider using a higher bond than the minimum. Since the result of the data asserted can have a lot of funds associated with it, a higher bond will increase the security and confidence the data is accurate.

In LM_PC_Staking_v1.sol, users stake assets and receive rewards over time based on their proportion of total staked assets and the set reward and duration.

The amount of rewards a user is owed is calculated for the most part in the functions _calculateRewardValue() and _earned():

///@dev This is the heart of the algorithm
/// The reward Value is the accumulation of all the rewards a user would get for a single token if they had staked at the beginning of the lifetime of this contract
/// A "single" reward value or with the lack of a better word "reward period" is the rewardRate (so the rewards per second for the whole contract)
/// multiplied by the time period it was active and dividing that with the total supply
/// This "single" value is essentially what a single token would have earned in that time period
function _calculateRewardValue() internal view returns (uint) {
    //In case the totalSupply is 0 the rewardValue doesnt change
    if (totalSupply == 0) {
        return rewardValue;
    }

    return (_getRewardDistributionTimestamp() - lastUpdate) //Get the time difference between the last time it was updated and now (or in case the reward period ended the rewardEnd timestamp)
        * rewardRate //Multiply it with the rewardrate to get the rewards distributed for all of the stakers together
        * 1e18 // for the later division we need a value to compensate for the loss of precision. This value will be counteracted in earned()
        / totalSupply //divide it by the totalSupply to get the rewards per token
        + rewardValue; //add the old rewardValue to the new "single" rewardValue
}

Reference: LM_PC_Staking_v1.sol#L237-L253

 /// @dev internal function to calculate how much a user earned for their stake up to this point
/// Uses the difference between the current Reward Value and the reward value when the user staked their tokens
/// in combination with their current balance to calculate their earnings
function _earned(address user, uint providedRewardValue)
    internal
    view
    returns (uint)
{
    return (providedRewardValue - userRewardValue[user]) //This difference in rewardValues basically represents the time period between now and the moment the userRewardValue was created
        * _balances[user] // multiply by users balance of tokens to get their share of the token rewards
        / 1e18 // See comment in _calculateRewardValue();
        + rewards[user];
}

Reference: LM_PC_Staking_v1.sol#L262-L274

As mentioned in the comments of these functions, a value of 1e18 is multiplied in _calculateRewardValue() and divided out in _earned() in order to maintain precision.

However, this value of 1e18 is not sufficient to completely maintain precision when the total staked supply is sufficiently large relative to the current rewards rate. In the current state with reasonable values, there is a minor loss of precision. The value 1e18 has a fair bit of room to increase without overflow risk for reasonable values of rewardRate and totalSupply, and the larger this value is, the more precision that can be maintained

Remediations to Consider

Increase this precision value to e.g. 1e36 to reduce loss of precision for user rewards.

In the case where the Orchestrator itself is set to have the OrchestratorOwner role, it can end up leading to modules calling functions on other modules as if they were the OrchestratorOwner. This is because modules are able to call executeTxFromModule(), allowing calls with arbritrary data to be made by the orchestrator:

/// @inheritdoc IModuleManagerBase_v1
function executeTxFromModule(address to, bytes memory data)
    external
    virtual
    onlyModule
    returns (bool, bytes memory)
{
    bool ok;
    bytes memory returnData;

    (ok, returnData) = to.call(data);

    return (ok, returnData);
}

Reference: ModuleManagerBase_v1.sol#L258-L271

This allows for modules to be added that could override the authorization scheme, potentially allowing anyone to call permissioned functions on other modules with any data.

Remediations to Consider

Add checks in authorizers to ensure that the orchestrator cannot have the ORCHESTRATOR_OWNER_ROLE.

In the FM_BC_Bancor_Redeeming_VirtualSupply_v1 init() function setup of collateral and issuance token is performed. To guarantee that these tokens have a proper number of decimals, their decimals are compared and validated within _setIssuanceToken().

However, _setIssuanceToken() is called and validation is performed before collateralTokenDecimals is set. As a result, at the time of expression evaluation, collateralTokenDecimals is 0. Therefore, it is possible to set an issuance token with a number of decimals less than the collateralTokenDecimals. This is not according to the implementation intention and may cause incorrect results.

Remediations to consider:

• Change the order of operations in init(), and initialize collateralTokenDecimals before invoking _setIssuanceToken()

// Set issuance token. This also caches the decimals
_setIssuanceToken(address(_issuanceToken));

// Set accepted token
_token = IERC20(_acceptedToken);

// Cache token decimals for collateral
collateralTokenDecimals = IERC20Metadata(address(_token)).decimals();

In the IFM_BC_Bancor_Redeeming_VirtualSupply_v1 following functions are meant to be accessible by the Orchestrator owner and Manager:

• setReserveRatioForBuying()
• setReserveRatioForSelling()

In the IRedeemingBoundingCurveBase_v1 both Orchestrator owner and Manager should have access to the following functions:

• openSell()
• closeSell()
• setSellFee()

In the IBondingCurveBase_v1 Orchestratror owner and Manager should be able to invoke:

• openBuy()
• closeBuy()
• setBuyFee()

However in corresponding implementations for these interfaces none of the functions listed above is accessible to the Manager.

Remediations to consider

• Update implementation contracts to use different access control modifier which allows Manager to invoke these functions, or
• Update natspec and remove mention of Manager capabilities in reference to these functions.

BondingCurveBase_v1.sol is intended to be a funding manager, and declares that it supports the IFundingManager interface:

...
abstract contract BondingCurveBase_v1 is IBondingCurveBase_v1, Module_v1 {
    function supportsInterface(bytes4 interfaceId)
        public
        view
        virtual
        override(Module_v1)
        returns (bool)
    {
        return interfaceId == type(IBondingCurveBase_v1).interfaceId || interfaceId == type(IFundingManager_v1).interfaceId
            || super.supportsInterface(interfaceId);
    }
...

Reference: BondingCurveBase_v1.sol#L35-L46

However, it does not actually implement the IFundingManager interface, and has no implemented or abstract functions that it requires. If downstream contracts do not implement the expected functions while indicating that they do, it may cause these contracts to be added as funding managers and pass the checks in the orchestrator when they shouldn’t:

 ...
 bytes4 fundingManagerInterfaceId = type(IFundingManager_v1).interfaceId;
  if (
      ERC165Checker.supportsInterface(
          fundingManagerContract, moduleInterfaceId
      )
          && ERC165Checker.supportsInterface(
              fundingManagerContract, fundingManagerInterfaceId
          )
  ) {
  ...

Reference: Orchestrator_v1.sol#L241-L249

Remediations to Consider

• Add abstract versions of the token() and transferOrchestratorToken() functions to BondingCurveBase_v1 to ensure any downstream contracts implement this functions, or
• Remove out support for IFundingManager interface in BondingCurveBase_v1 and leave to child contracts to properly declare that support on lower level in the contract inheritance chain.

In Orchestrator_v1, there are 3 required modules, the funding manager, payment processor, and the authorizer, which are set up on initialization and can be directly queried. Although additional modules can be added and removed, these modules are special as they always need to be present, but they can be replaced by the orchestrator owner with a special but similar setter for each.

/// @inheritdoc IOrchestrator_v1
function setFundingManager(IFundingManager_v1 fundingManager_)
    external
    onlyOrchestratorOwner
{
    address fundingManagerContract = address(fundingManager_);
    bytes4 moduleInterfaceId = type(IModule_v1).interfaceId;
    bytes4 fundingManagerInterfaceId = type(IFundingManager_v1).interfaceId;
    if (
        ERC165Checker.supportsInterface(
            fundingManagerContract, moduleInterfaceId
        )
            && ERC165Checker.supportsInterface(
                fundingManagerContract, fundingManagerInterfaceId
            )
    ) {
            ///@audit: notice the new funding manager is added before the prior was removed
        addModule(address(fundingManager_));
        removeModule(address(fundingManager));
        fundingManager = fundingManager_;
        emit FundingManagerUpdated(address(fundingManager_));
    } else {
        revert Orchestrator__InvalidModuleType(address(fundingManager_));
    }
}

Reference: Orchestrator_v1.sol#L234-L257

However, when these functions are called, the new module is attempted to be added via the addModule() function before the prior one is removed. In most cases, this is fine, but in the case where the number of modules is at the MAX_MODULE_AMOUNT of 128, this causes a revert since addModule() calls moduleLimitNotExceeded before adding:

/// @inheritdoc IModuleManagerBase_v1
function addModule(address module)
    public
    __ModuleManager_onlyAuthorized
    moduleLimitNotExceeded
    isNotModule(module)
    validModule(module)
{
    _commitAddModule(module);
}

Reference: ModuleManagerBase_v1.sol#L180-L189

modifier moduleLimitNotExceeded() {
    if (_modules.length >= MAX_MODULE_AMOUNT) {
        revert ModuleManagerBase__ModuleAmountOverLimits();
    }
    _;
}

Reference: ModuleManagerBase_v1.sol#L77-L82

This prevents updating one of these special modules without first removing another module if the module limit is reached.

Remediations to Consider

Reorder the calls in setAuthorizer(), setFundingManager() and setPaymentProcessor() to call removeModule() before addModule() to cover this edge case.

In Orchestrator_v1.sol, when it is initialized via the init() function, it adds its fundingManager, paymentProcessor, and authorizer module directly, calling __ModuleManager_addModule() for each:

fundingManager = fundingManager_;
authorizer = authorizer_;
paymentProcessor = paymentProcessor_;

governor = governor_;

// Add necessary modules.
// Note to not use the public addModule function as the factory
// is (most probably) not authorized.
__ModuleManager_addModule(address(fundingManager_));
__ModuleManager_addModule(address(authorizer_));
__ModuleManager_addModule(address(paymentProcessor_));

Reference: Orchestrator_v1.sol#L115-L126

However, if any of these modules is changed it calls it’s own function that explicitly requires that the replacement module supports the required interface:

function initiateSetFundingManagerWithTimelock(
    IFundingManager_v1 fundingManager_
) external onlyOrchestratorOwner {
    address fundingManagerContract = address(fundingManager_);
    bytes4 moduleInterfaceId = type(IModule_v1).interfaceId;
    bytes4 fundingManagerInterfaceId = type(IFundingManager_v1).interfaceId;
    if (
        ERC165Checker.supportsInterface(
            fundingManagerContract, moduleInterfaceId
        )
            && ERC165Checker.supportsInterface(
                fundingManagerContract, fundingManagerInterfaceId
            )
    ) {
        _initiateAddModuleWithTimelock(address(fundingManager_));
        _initiateRemoveModuleWithTimelock(address(fundingManager));
    } else {
        revert Orchestrator__InvalidModuleType(address(fundingManager_));
    }
}

Reference: Orchestrator_v1.sol#L252-L271

Since the interface is not directly checked, it could lead to improper initial setup, using a contract that does may implement the intended functions.

Remediations to Consider

Check if the required modules support the required interfaces, as is done when updating these modules, to prevent errors and be more consistent.

ModuleManagerBase_v1.sol is inherited by Orchestrator_v1.sol, and manages the modules it controls, ensuring they are valid and there are not duplicates among other responsibilities. However, when adding a module, it checks if the new module is valid by calling the validModule modifier, which calls _ensureValidModule():

function __ModuleManager_addModule(address module)
        internal
        isNotModule(module)
        validModule(module)
    {
        _commitAddModule(module);
    }

Reference: ModuleManagerBase_v1.sol#L158-L164

modifier validModule(address module) {
    _ensureValidModule(module);
    _;
}

Reference: ModuleManagerBase_v1.sol#L60-L63

function _ensureValidModule(address module) private view {
        ///@audit: it does not check if the address supports the IModule interface which the protocol expects
    if (module == address(0) || module == address(this)) {
            revert ModuleManagerBase__InvalidModuleAddress();
        }
    }

Reference: ModuleManagerBase_v1.sol#L316-L320

However, _ensureValidModule() only checks if the address is valid and does not ensure the new module supports the IModule interface. This could lead to errors if the protocol expects to interact with these functions on this module.

Remediations to Consider

In _ensureValidModule(), check if the new module supports the IModule_v1 interface to ensure the module can be interacted with as expected.

In the OptimisticOracleIntegrator, the assertionLiveness setting defines the time period within which an assertion is open for dispute (in seconds). This configuration setting can be updated on initialization in __OptimisticOracleIntegrator_init() or using setDefaultAssertionLiveness(), which is accessible only to the orchestrator owner.

Currently, there is only validation that the provided value is not 0. However, if assertionLiveness is set to some relatively small value, such as 1 second, that would not allow meaningful system operation.

Remediations to Consider

Consider adding validation for a valid minimum period for assertionLiveness config changes.

In LibMetadata, a module’s metadata is hashed to create a “unique” id or identifier():

 /// @dev Returns the identifier for given metadata.
/// @param metadata The metadata.
/// @return The metadata's identifier.
function identifier(IModule_v1.Metadata memory metadata)
    internal
    pure
    returns (bytes32)
{
    return keccak256(
        abi.encodePacked(
            metadata.majorVersion, metadata.url, metadata.title
        )
    );
}

Reference: LibMetadata.sol#L14-L27

However, this identifier may not be unique since the metadata values are encoded with abi.encodePacked() and the values url and title are strings which are dynamic values. When dynamic values are encoded with abi.encodePacked(), the first word that determines its length is removed, and only the content is encoded. This has the effect of when multiple dynamic values are encoded and packed next to each other; different values can encode to the same thing. For example: abi.encodePacked(”someUrl”, “module title”) == abi.encodePacked(”some”, “Urlmodule title”)

This can lead to modules being created in the ModuleFactory with unintended metadata, which could invalidate URLs and titles.

Remediations to Consider

Instead of using abi.encodePacked() to hash the metadata values in the identifier, use abi.encode() to preserve the length data and ensure all unique metadata values also have a unique identifier with no collisions.

Throughout the protocol there are multiple external calls to other contracts that are expected to support particular interfaces. However, some interfaces describe functions that are clearly always intended to be getter view functions, but are not set as view. This could cause contracts that are setup within the protocol that implement these functions but when called have the ability to take over execution in unintended ways.

Remediations to Consider

Make sure that each interface function which is intended to be read only is explicitly set as view.

In the AUT_TokenGated_Roles_v1, grantTokenRoleFromModule() is used to configure a specific role. While parent contract AUT_Roles_v1 features capabilities to grantRoleFromModule() and revokeRoleFromModule(), token gated child contract does not feature a specific function for removing the token gated role.

However, if parent revokeRoleFromModule() is utilized thresholdMap[thresholdId] associated with the particular role and token will remain set and potentially reused if role is granted again to the same token in the future.

Remediations to Consider

• Override revokeRoleFromModule() in the AUT_TokenGated_Roles_v1 to perform necessary cleanup actions in addition to those performed in the parent contract’s implementation.

In the AUT_TokenGated_Roles_v1, _grantRole() implementation performs check if provided address is a Token.

if (isTokenGated[role]) {
    try TokenInterface(account).balanceOf(address(this)) {}
    catch {
        revert Module__AUT_TokenGated_Roles__InvalidToken(account);
    }
}

However, the current check will pass for any EOA address or for any other contract with a fallback() method, which will not result in a revert/exception that will be caught in the catch block.

Remediations to Consider

Consider removing this check, which may result in a false sense of security, or making this guard condition more restrictive.

In the PP_Streaming_v1 contract, when processPayments() is invoked, pending orders are iterated through and processed individually. In addition to their corresponding data attributes, each order's _walletId is initialized based on the current counter specific to the particular client recipient combination.

...
_walletId = numVestingWallets[address(client)][_recipient] + 1;

_addPayment(
    address(client),
    _recipient,
    _amount,
    _start,
    _dueTo,
    _walletId
);

emit PaymentOrderProcessed(
    address(client),
    _recipient,
    _amount,
    _start,
    _dueTo,
    _walletId
);

When validation passes, the specific counter is incremented, and the StreamingPaymentAdded() event is emitted.

However, when validation fails in the _addPayment() function, the function does not revert but emits an InvalidStreamingOrderDiscarded() event. In this case, it doesn't increment a specific counter on which the _walletId value is based.

Because of this slightly different behavior, it is possible for multiple orders for the same recipient to fail, and they will emit multiple PaymentOrderProcessed() events with the same _walletId value. Moreover, a case exists where multiple failures and a single successfully processed order each emit a PaymentOrderProcessed event with the same walletId value. As a result, off-chain tracking and monitoring tools may not operate properly unless aware of this edge case.

Remediations to consider

• Update PaymentOrder data struct to maintain unique orderId across various stages of processing, or
• Increment numVestingWallets[client][_paymentReceiver] even when validation fails in _addPayment().

In the ModuleManagerBase_v1 contract, the _isModule variable is defined without an explicit visibility specifier. The default visibility for this variable, in that case, would be internal.

mapping(address => bool) _isModule;

As a result, contracts inheriting from the ModuleManagerBase_v1 would be able to directly manipulate _isModule mapping instead of using or overriding corresponding functions from the ModuleManagerBase_v1.

Remediations to consider

Update the variable definition to include a private visibility specifier so it cannot be directly updated without the associated functionality present in ModuleManagerBase_v1 being executed.

In some areas there is type casting from one type to another, that isn’t necessarily. For example in Orchestrator_v1.sol’s initiateSetFundingManagerWithTimelock() function, there is a local fundingManagerContract address variable created that is casted from IFundingManager_v1 to an address, but it is never used, and it is continually directly casted to an address where required:

function initiateSetFundingManagerWithTimelock(
    IFundingManager_v1 fundingManager_
) external onlyOrchestratorOwner {
        ///@audit: local value is created, but never used
    address fundingManagerContract = address(fundingManager_);
    bytes4 moduleInterfaceId = type(IModule_v1).interfaceId;
    bytes4 fundingManagerInterfaceId = type(IFundingManager_v1).interfaceId;
    if (
        ERC165Checker.supportsInterface(
            fundingManagerContract, moduleInterfaceId
        )
            && ERC165Checker.supportsInterface(
                fundingManagerContract, fundingManagerInterfaceId
            )
    ) {
            ///@audit: could be used here instead of recasting
        _initiateAddModuleWithTimelock(address(fundingManager_));
        _initiateRemoveModuleWithTimelock(address(fundingManager));
    } else {
            ///@audit: as well as here
        revert Orchestrator__InvalidModuleType(address(fundingManager_));
    }
}

Reference: Orchestrator_v1.sol#L252-L271

A Similar issue is found in initiateSetAuthorizerWithTimelock() and initiateSetPaymentProcessorWithTimelock().

Remediations to Consider

Use the local variable setup in these functions to prevent recasting and duplicating code.

In the IOptimisticOracleIntegrator, assertionResolvedCallback() and assertionDisputedCallback() are declared explicitly, which is unnecessary since IOptimisticOracleIntegrator inherits from the OptimisticOracleV3CallbackRecipientInterface which declares these same callback functions.

Consider removing the explicit declaration of assertionResolvedCallback() and assertionDisputedCallback() from the IOptimisticOracleIntegrator.

The claimRewards() function implementation is present in the LM_PC_Staking_v1 contract. However, the corresponding interface, ILM_PC_Staking_v1, lacks this function declaration.

• In ILM_PC_KPIRewarder_v1

  error Module__LM_PC_KPIRewarder_v1__InvalidTargetValue();
  

• In IModuleManagerBase_v1

  error ModuleManagerBase__ModulesNotConsecutive();
  

• In IFM_BC_Bancor_Redeeming_VirtualSupply_v1

  error Module__FM_BC_Bancor_Redeeming_VirtualSupply__InvalidDepositAmount();
  

• In ILM_PC_Staking_v1 the following event is defined but never used

  /// @notice Event emitted when the reward duration is updated.
  event RewardsDurationUpdated(uint newDuration);
  

• In ILM_PC_KPIRewarder_v1, the following events may use indexed attributes
  • FeeFundsDeposited (token)
  • StakeEnqueued (user)
  • StakeDequeued (user)
  • KPICreated (KPI_Id)

• In IInverterBeacon_v1.sol, it imports IModule_v1 and IOrchestrator_v1

• In LM_PC_KPIRewarder_v1

  - import "forge-std/console.sol";
  
      import {
          ILM_PC_Staking_v1,
          LM_PC_Staking_v1,
          SafeERC20,
          IERC20,
          ERC20PaymentClientBase_v1,
  -	    IERC20PaymentClientBase_v1,
  -	    ReentrancyGuard
      } from "./LM_PC_Staking_v1.sol";
  
    import {
       IOptimisticOracleIntegrator,
       OptimisticOracleIntegrator,
  -    OptimisticOracleV3CallbackRecipientInterface,
  -    OptimisticOracleV3Interface,
  -    ClaimData
    } from
      "src/modules/logicModule/abstracts/oracleIntegrations/UMA_OptimisticOracleV3/OptimisticOracleIntegrator.sol";
  

• In ILM_PC_KPIRewarder_v1

  import {
      ILM_PC_Staking_v1,
      LM_PC_Staking_v1,
      SafeERC20,
      IERC20,
      IERC20PaymentClientBase_v1,
      ReentrancyGuard
  } from "src/modules/logicModule/LM_PC_Staking_v1.sol";
  
  import {
      IOptimisticOracleIntegrator,
      OptimisticOracleIntegrator,
      OptimisticOracleV3CallbackRecipientInterface,
      OptimisticOracleV3Interface,
      ClaimData
  } from
      "@lm/abstracts/oracleIntegrations/UMA_OptimisticOracleV3/OptimisticOracleIntegrator.sol";
  

• In OptimisticOracleIntegrator

  - import {OptimisticOracleV3CallbackRecipientInterface} from "@lm/abstracts/oracleIntegrations/UMA_OptimisticOracleV3/optimistic-oracle-v3/interfaces/OptimisticOracleV3CallbackRecipientInterface.sol";
  - import {ERC165Checker} from "@oz/utils/introspection/ERC165Checker.sol";
  

• In ERC20PaymentClientBase_v1

  import {
           Module_v1, 
  -    ContextUpgradeable
  } from "src/modules/base/Module_v1.sol";
  

• In BondingCurveBase_v1

  import {IOrchestrator_v1} from
      "src/orchestrator/interfaces/IOrchestrator_v1.sol";
  

• In FM_BC_Bancor_Redeeming_VirtualSupply_v1

  import {IBondingCurveBase_v1} from
      "@fm/bondingCurve/interfaces/IBondingCurveBase_v1.sol";
  import {IRedeemingBondingCurveBase_v1} from
      "@fm/bondingCurve/interfaces/IRedeemingBondingCurveBase_v1.sol";
  

None of the functions from the src/modules/fundingManager/bondingCurve/formulas/Utils.sol are being used even though BancorFormula contract inherits from Utils.

Consider removing Utils.sol and reference to it in the BancorFormula contract.

Function _calculateNetAmountAndFee() is not used. Consider removing it.

Bool variables do not need to be compared with literal true/false values.

For example:

// replace
if (buyIsOpen == false) {
// with
if (!buyIsOpen) {

Instances of this present in multiple contracts:

• BondingCurveBase_v1 - buyingIsEnabled() , or _openBuy(), _closeBuy()
• RedeemingBondingCurveBase_v1 - sellingIsEnabled() , or _openSell(), _closeSell()
• FM_BC_Bancor_Redeeming_VirtualSupply_v1 - init()

In Module_v1 and OrchestratorFactory_v1, _dependencyInjectionRequired() function has following implementation:

function _dependencyInjectionRequired(bytes memory dependencyData)
    internal
    view
    returns (bool)
{
    try this.decoder(dependencyData) returns (bool) {
        return this.decoder(dependencyData);
    } catch {
        return false;
    }
}

Consider updating to the following to avoid duplicate decoder() function calls

function _dependencyInjectionRequired(bytes memory dependencyData)
    internal
    view
    returns (bool)
{
    try this.decoder(dependencyData) returns (bool requirement) {
        return requirement;
    } catch {
        return false;
    }
}

In ModuleManagerBase_v1, the following variable is defined but never used. Consider removing it.

/// @dev Mapping of modules and access control roles to accounts and
///      whether they holds that role.
/// @dev module address => role => account address => bool.
///
/// @custom:invariant Modules can only mutate own account roles.
/// @custom:invariant Only modules can mutate not own account roles.
/// @custom:invariant Account can always renounce own roles.
/// @custom:invariant Roles only exist for enabled modules.
mapping(address => mapping(bytes32 => mapping(address => bool))) private
    _moduleRoles;

In multiple interfaces events are declared with indexed attribute for uint variables representing different kinds of amounts. Indexing these attributes is not useful since rarely will they be used for searching. However, indexed attributes will result in less gas efficient event emission.

Therefore, consider removing indexed attributes across codebase for uint parameters in events (unless uint parameter acts as enum).

In BondingCurveBase_v1:_buyOrder() there are two instances of reading same cross contract variable.

// #1 instance 
__Module_orchestrator.fundingManager().token().safeTransferFrom(
    _msgSender(), address(this), _depositAmount
);

// #2 instance
_processProtocolFeeViaTransfer(
    collateralTreasury,
    __Module_orchestrator.fundingManager().token(),
    protocolFeeAmount
);

In RedeemingBondingCurveBase_v1:_sellOrder() there are three instances of reading same cross contract variable.

// #1 instance 
if (
    (collateralRedeemAmount + projectCollateralFeeCollected)
        > __Module_orchestrator.fundingManager().token().balanceOf(
            address(this)
        )
) {

// #2 instance
_processProtocolFeeViaTransfer(
    collateralreasury,
    __Module_orchestrator.fundingManager().token(),
    protocolFeeAmount
);

// #3 instance
__Module_orchestrator.fundingManager().token().transfer(
    _receiver, collateralRedeemAmount
);

Consider caching call __Module_orchestrator.fundingManager().token() for more gas efficient execution.

The LM_PC_Staking_v1 module's implemented functionality does not properly handle fee-on-transfer tokens, so they are not currently supported. For example, see stake() function.

In Governor_v1, the COMMUNITY_MULTISIG_ROLE can directly call forceUpgradeBeaconAndRestartImplementation() which upgrades the implementation of a InverterBeacon contract, and thus upgrades all modules that use it as a beacon:

/// @inheritdoc IGovernor_v1
function forceUpgradeBeaconAndRestartImplementation(
    address beacon,
    address newImplementation,
    uint newMinorVersion
)
    external
    onlyRole(COMMUNITY_MULTISIG_ROLE)
    accessibleBeacon(beacon)
    validAddress(newImplementation)
{
    IInverterBeacon_v1(beacon).upgradeTo(
        newImplementation, newMinorVersion, true
    );
    emit BeaconForcefullyUpgradedAndImplementationRestarted(
        beacon, newImplementation, newMinorVersion
    );
}

Reference: Governor_v1.sol#L319-L336

This upgrade is not opt-in and may give users little time to react to the upgrade, depending on how the addresses with the COMMUNITY_MULTISIG_ROLE are set. Upgrades can also be set by users with either the TEAM_MULTISIG_ROLE or COMMUNITY_MULTISIG_ROLE, but it is set with a timelock delay before the upgrade can execute, which allows users to react. Modules can also be shutdown by either role, preventing interaction, with the intent being to mitigate the effect of discovered vulnerabilities so they can be patched.

Ensuring these permissioned wallets are themselves behind a timelock, are sufficiently secure, and transparent is advised. Users should become aware of these potentials before deciding to interact with protocols using this system.

In the PP_Streaming_v1 and PP_Simple_v1 modules, token transfers are performed to external receivers. If token contracts support callbacks to external receivers, these receivers may intentionally cause order batches to revert.

Depending on how these failures are handled in concrete payment client implementation, this may block all pending payouts or cause some form of DoS, in which case it would be necessary to retry processPayments() calls.

The OrchestratorOwner role has the most control over the protocol, and depending on the modules set, it could steal users' assets by adjusting values by calling privileged functions. For the most part, privileged functions are moderated by timelocks that allow users to react to malicious updates, but there are functions in some current modules without these guards that could be taken advantage of. Users should be aware that the protocol is not completely free of centralization issues, and trust in privileged roles is required.