Olympus DAO contest - Jeiwan's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Heart.sol#L92-L109

Vulnerability details

Impact

The beat function can be called multiple times within the same block or in next blocks without triggering the Heart_OutOfCycle error. The function is expected to be called not often than once in frequency times (a related test case). However, if a beat gets delayed for a longer period or observation frequency was reduced, further beats will be able to circumvent the restriction. As a result, the caller of beat (which can be any address) will get extra amount of reward tokens for calling beat multiple times in a block.

Proof of Concept

Here's a Forge test case that reproduces the issue (part of HeartTest):

function testCorrectness_beatOutOfCycle() public {
    uint256 frequency = heart.frequency();
    vm.warp(block.timestamp + frequency);
    uint256 startBalance = rewardToken.balanceOf(address(this));

    heart.beat();

    vm.warp(block.timestamp + frequency);
    heart.beat();

    vm.warp(block.timestamp + frequency * 5);
    heart.beat();
    heart.beat();

    vm.warp(block.timestamp + 1);
    heart.beat();
    heart.beat();
    heart.beat();

    uint256 endBalance = rewardToken.balanceOf(address(this));
    assertEq(endBalance, startBalance + heart.reward() * 7);
}

First two beats work as expected. The third beat is delayed, which sets lastBeat to an older timestamp. This makes it possible to call beat multiple beats in the same block or call beat again in the next block. In the end, the caller was rewarded 7 times for calling beat, including the out-of-cycle calls. Since calling beat multiple times in a block triggers changes in the Price policy and the Operator contract only once (during the first call in a block), the rest calls get rewarded for doing no useful work.

Another situation than can cause the issue is when price observation frequency was reduced:

function testCorrectness_beatOutOfCycle2() public {
    // frequency was set to 8 hours initially
    uint256 frequency = heart.frequency();
    vm.warp(block.timestamp + frequency);
    uint256 startBalance = rewardToken.balanceOf(address(this));

    heart.beat();

    price.changeObservationFrequency(2 hours);

    vm.warp(block.timestamp + frequency);
    heart.beat();
    heart.beat();
    heart.beat();
    heart.beat();

    uint256 endBalance = rewardToken.balanceOf(address(this));
    assertEq(endBalance, startBalance + heart.reward() * 5);
}

In this case, the delay is shorter and equals to the old frequency value (it can be even shorter to allow only one extra beat). Such situation can happen when observation frequency was reduced but those who trigger beat were not aware or notified about that (or they delayed the next call intentionally). As a result, they'll be able to get extra reward by calling beat multiple times in a block.

Tools Used

Recommended Mitigation Steps

In the beat function, set lastBeat to block.timestamp.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/Kernel.sol#L252-L253 https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/INSTR.sol#L52

Vulnerability details

Impact

After contracts are deployed and initialized, the admin address in Kernel contract can only be set to a contract. Granting and revoking roles will be possible to do only via a contract, which looks like an unintended behavior since these operations cannot be performed via governance (the governance contract is designed to be the only executor).

Proof of Concept

Admin address can be changed to any address (EOA or contract) in the executeAction function in Kernel: https://github.com/code-423n4/2022-08-olympus/blob/main/src/Kernel.sol#L252-L253

This piece explicitly allows EOA addresses since the other actions in the function (besides ChangeExecutor) are checked to have only a contract as the target (see ensureContract function calls in the other actions). This, and the fact that roles cannot be managed via governance, leads to the conclusion that an admin is designed to be an EOA.

However, in the store function in INSTR, action target can only be a contract: https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/INSTR.sol#L52

After the contracts are deployed, INSTR will be the only contract that's allowed to call Kernel.executeAction: https://github.com/code-423n4/2022-08-olympus/blob/main/src/scripts/Deploy.sol#L220

Thus, there will be no way to change admin to an EOA. If admin needs to be an EOA, the INSTR contract needs to be patched and re-deployed to allow non-contract targets.

Tools Used

Recommended Mitigation Steps

Allow EOA addresses as instruction targets or disallow non-contract admin addresses.

• Finding: Constructor parameter are not validated Severity: QA Description: Constructor parameters are not validated at:

  • https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/RANGE.sol#L77 To mitigate the risk of misconfiguration, it's recommended to validate tokens_ and rangeParams_ parameters (the former is used to set immutable variables, which cannot be modified after being set incorrectly in constructor).

• Finding: Misconfiguration risk due to usage of arrays to pass function arguments Severity: QA Description: The usage of arrays when passing multiple arguments to functions can cause misconfiguration issues because deployer/ user has to ensure the order of array elements is correct when calling a function. It's recommended to use parameters structures in these cases:

  • https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/RANGE.sol#L79-L80 For tokens_ and rangeParams_. Passing tokens in a wrong order might result in an unusable contract.
  • https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L96-L97 For tokens_ and configParams. Passing tokens in a wrong order might result in an unusable contract. configParams contains many values that are hard to read/track in the code since they're referenced by their index in the array, not their name.

  Poor readability of array arguments forced developers to add the comments with argument names and their order.

  An example of a parameters structure:

  • https://github.com/Uniswap/v3-periphery/blob/main/contracts/NonfungiblePositionManager.sol#L128
  • https://github.com/Uniswap/v3-periphery/blob/75f3b72b4412b41e31c2a2370bb52d55f99ec717/contracts/interfaces/INonfungiblePositionManager.sol#L79-L91

• Finding: Poor validation of "debt buffer" arguments Severity: QA Description: These function arguments are not validated according to the documentation:

  • cushionDebtBuffer at https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L97
  • debtBuffer_ at https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L527

  In both of these cases, the passed value is checked to be lower than 10000, however, in MarketParams documentation in IBondAuctioneer:

  Minimum is the greater of 10% or initial max payout as a percentage of capacity. The value must be > 10% but can exceed 100% if desired.

  Also, the documentation says:

  If the value is too small, the market will not be able function normally and close prematurely.

  However, there's no minimal value check in the above functions.

• Finding: Unused function Severity: QA Description: fromRole in KernelUtils.sol is not used:

  • https://github.com/code-423n4/2022-08-olympus/blob/main/src/utils/KernelUtils.sol#L26

• Finding: Unnecessary calldata to memory copying Severity: Gas optimization Description: Copying function arguments to memory is not necessary when the arguments are not expected to be modified in the function. It's recommended to use calldata storage type instead of memory in these cases:
  • https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L205
  • https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/BondCallback.sol#L152
  • https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/PriceConfig.sol#L45