Olympus DAO contest Findings &amp; Analysis Report

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L64-L72
https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/TreasuryCustodian.sol#L42-L48

Leaving as High severity as this shows a clear path to loss of funds.

[H-03] TRSRY: front-runnable `setApprovalFor`

Submitted by zzzitron, also found by berndartmueller, csanuragjain, pashov, Ruhum, sorrynotsorry, and Trust

An attacker may be able to withdraw more than intended

Proof of Concept

Let’s say Alice had approval of 100. Now the treasury custodian reduced the approval to 50. Alice could frontrun the setApprovalFor of 50, and withdraw 100 as it was before. Then withdraw 50 with the newly set approval. So the alice could withdraw 150.

// modules/TRSRY.sol

 63     /// @notice Sets approval for specific withdrawer addresses
 64     function setApprovalFor(
 65         address withdrawer_,
 66         ERC20 token_,
 67         uint256 amount_
 68     ) external permissioned {
 69         withdrawApproval[withdrawer_][token_] = amount_;
 70
 71         emit ApprovedForWithdrawal(withdrawer_, token_, amount_);
 72     }

The TreasuryCustodian simply calls the setApprovalFor to grant Approval.

 41
 42     function grantApproval(
 43         address for_,
 44         ERC20 token_,
 45         uint256 amount_
 46     ) external onlyRole("custodian") {
 47         TRSRY.setApprovalFor(for_, token_, amount_);
 48     }

Recommended Mitigation Steps

Instead of setting the given amount, one can reduce from the current approval. By doing so, it checks whether the previous approval is spend.

0xean (judge) increased severity to High and commented:

Understood. Will change the logic to increase/decrease allowances.

I think this vulnerability should be a high severity as it opens up the possibility of a direct loss of funds in the amount of up to the previous approval amount. Upgrading to High.

@ind-igo - Not sure if you deleted your comment, but that context is useful. Happy to take another look here.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L548
https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L711
https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/RANGE.sol#L133
https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/RANGE.sol#L145
https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/RANGE.sol#L185
https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L780

I did, I just thought it was unnecessary to evaluate the issue. I was just saying that the context of the code is that it is not intended to be used to approve an EOA/multisig, but instead used to approve governance-voted contracts to access treasury funds, in order to deposit into yield contracts or whatever. But I don’t think it’s very relevant to this, as the code is still faulty and exploitable in an extreme case. I already have made this remediation as well, so all good.

Medium Risk Findings (32)

[M-01] `Operator::setReserveFactor` doesn’t check if bond market should be changed

Submitted by rvierdiiev

Operator::setReserveFactor sets new reserveFactor value. This parameter is used in fullCapacity function to calculate how much capacity is available by high/low side. Then calculated capacity is used by Range module inside regenerate function to set the threshold of capacity for both sides of market. Then in Range::updateCapacity function this threshold is checked to understand if the wall should be down and the bond market should be closed.

Changing this value means that the capacity of sides has changed and the sides should be regenarated to include this changes.

Proof of Concept

Recommended Mitigation Steps

Call this after the param updating.
_regenerate(true); _regenerate(false;

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/TRSRY.sol#L110
https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/TRSRY.sol#L99

Forcing a regeneration when the reserveFactor is updated could cause unintended regeneration if a wall is currently down. A better approach may be to conditionally regenerate each side if they are active.

Oighty (Olympus) acknowledged and commented:

After discussing with the team more, we are going to leave this as-is. It is more flexible to not regenerate the side in this function. With the current implementation, the guardian can determine if the change should go into effect on the next regen, or if it should happen immediately. To enable immediately, they can manually call regenerate.

[M-02] Solmate `safetransfer` and `safetransferfrom` does not check the codesize of the token address, which may lead to fund loss

Submitted by djxploit, also found by brgltd

In getloan() and replayloan(), the safetransfer and safetransferfrom doesn’t check the existence of code at the token address. This is a known issue while using solmate’s libraries.
Hence this may lead to miscalculation of funds and may lead to loss of funds , because if safetransfer() and safetransferfrom() are called on a token address that doesn’t have contract in it, it will always return success, bypassing the return value check. Due to this protocol will think that funds has been transferred and successful , and records will be accordingly calculated, but in reality funds were never transferred.
So this will lead to miscalculation and possibly loss of funds

Proof of Concept

Recommended Mitigation Steps

Use openzeppelin’s safeERC20 or implement a code existence check.

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L195-L268

Confirmed. Will implement this. Thank you.

[M-03] RBS may redeploy funds automatically if price stays above or below wall for longer than `_config.regenWait`

Submitted by 0x52

Loss of treasury funds.

Proof of Concept

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/RANGE.sol#L133-L139

if (capacity_ < _range.high.threshold && _range.high.active) {
    // Set wall to inactive
    _range.high.active = false;
    _range.high.lastActive = uint48(block.timestamp);


    emit WallDown(true, block.timestamp, capacity_);
}

_range.high.lastActive and _range.low.lastActive are only updated in RANGE.sol when _range.x.capacity < _range.x.threshold and the _range.x.active == true. After this is tripped, _range.x.active will be set to false, meaning that _range.x.lastActive will not be updated again until the wall is regenerated and capacity is restored.

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L209-L214

    if (
        uint48(block.timestamp) >= RANGE.lastActive(true) + uint48(config_.regenWait) &&
        _status.high.count >= config_.regenThreshold
    ) {
        _regenerate(true);
    }

If 1) the price were to sustain outside of the range (high volatility for volatile asset, black swan for stable) for longer than config_.regenWait and 2) config_regenThreshold satisfies the following equation:

config_.regenThreshold <= _config.regenObserve - config_.regenWait / frequency

then status.high.count could be greater than config.regenThreshold. This would trigger more funds to be deployed even though the price never came back inside the wall price.

In this scenario the wall price would be far from the true price of the asset leading to loss of treasury funds as it buys/sell at prices well above/below market price.

Recommended Mitigation Steps

A check should be added to verify that the price is within the wall price before regenerating. Alternatively, config_.regenTheshold could be set to satisfy the following equation:

config_.regenThreshold > _config.regenObserve - config_.regenWait / frequency

This would eliminate the risk as _status.high.count >= config_.regenThreshold could never be true for a sustained period where current price is greater than the wall price.

Oighty (Olympus) disagreed with severity and commented:

This is valid. Our intended parameterization of the system would not be subject to this vulnerability, but it would be an issue if the system was incorrectly parameterized. Because it is an edge case, I’m not sure it is a high risk bug though.

Another potential fix is resetting the count to 0 and the observations array to new bool[](regenObserve) to clear out positive values from when a wall goes down. This could be done in the _updateCapacity() function by checking if the new capacity is under the threshold.

0xean (judge) decreased severity to Medium and commented:

Going to downgrade to Medium as the external dependency is a configuration that is not planned to be used by the sponsor.

[M-04] `OlympusGovernance#executeProposal`: reentrancy attack vulnerable function

Submitted by carlitox477, also found by cryptphi and ladboy233

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L265
https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L278-L288

Given that the activeProposal change is done before the for loop, if this function is call through one kernel.executeAction(instruction,target) we can call the same instructions (in the same order) again and again, which may or may not affect funds (depending on the instructions).

Proof of Concept

For instance, if we install a new module, and this module has a vulnerability (even intentional), the next steps can by trigger:

Call executeAction
This allow us to call kernel.executeAction in the for loop
executAction allow us to call _installModule
_installModule allow us to call newModule_.Init
By init we can call now executeProposal again (suppose that the init function interact with a previous vulnerable proxy contract to scam voters to vote in favour of this proposal as if it was a contract which is ok, and before calling executeProposal we change the implementation to allow this attack),

Recommended Mitigation Steps

Use nonReentrant modifier or move the line activeProposal = ActivatedProposal(0, 0); before the for loop.

fullyallocated (Olympus) confirmed and commented:

I don’t know if funds are going to be threatened, but this does allow for a re-entrancy. Warden is correct in resetting the active Proposal before the for loop based on the checks-effects-interactions code design pattern.

[M-05] Proposals overwrite

Submitted by 0x1f8b

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L167
https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L66

It is possible to overwrite proposals in certain circumstances. The method Governance.submitProposal doesn’t check if the proposalId (stored in a different contract) exists already as a valid proposal in getProposalMetadata.

Proof of Concept

If the project update the kernel module ”INSTR” and reconfigure proposals and call INSTR.store(instructions_);, the counter may return a proposalId of an existing proposal and overwrite an existing previous one.

This is due to the fact that the proposals are saved in a mapping of a contract that is not related to the one that returns the counters, and furthermore, they do not check that the record already exists.

        uint256 proposalId = INSTR.store(instructions_);
        getProposalMetadata[proposalId] = ProposalMetadata(
            title_,
            msg.sender,
            block.timestamp,
            proposalURI_
        );

Recommended Mitigation Steps

Store the proposal metadata in the same INSTR contract or ensure that the proposal doesn’t exist.

fullyallocated (Olympus) acknowledged, but disagreed with severity and commented:

Agreed with the validity of the circumstance, but it is contingent on us upgrading the contract in an unexpected way. Is the same as saying “if you upgrade a contract incorrectly it can break the dependencies”.

0xean (judge) decreased severity to Medium and commented:

Going to downgrade to medium based on some external requirements needing to be in place to be realized.

Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Function of the protocol could be impacted and there are external requirements.

[M-06] After endorsing a proposal, user can transfer votes to another user for endorsing the same proposal again

Submitted by rbserver, also found by 0x1f8b, Bahurum, csanuragjain, and yixxas

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/VOTES.sol#L9-L11
https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180-L201
https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L205-L236

The following comment indicates that the OlympusVotes contract is a stub for gOHM. Checking the gOHM contract at https://etherscan.io/token/0x0ab87046fBb341D058F17CBC4c1133F25a20a52f#code, the transfer and transferFrom functions are available.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/VOTES.sol#L9-L11

/// @notice Votes module is the ERC20 token that represents voting power in the network.
/// @dev    This is currently a substitute module that stubs gOHM.
contract OlympusVotes is Module, ERC20 {

Moreover, the documentation states that the vote redemption mechanism “exists to deter malicious behavior by ensuring users cannot transfer their voting tokens until after the proposal has been resolved”, which also indicates that the voting tokens are meant to be transferrable between users.

When the voting tokens are transferrable, one user can first use her or his votes to call the following endorseProposal function to endorse a proposal and then transfer these votes to another user. The other user can use these votes to endorse the same proposal again afterwards. Because of the double-endorsement, the (totalEndorsementsForProposal[proposalId_] * 100) < VOTES.totalSupply() * ENDORSEMENT_THRESHOLD condition can become true so the proposal can be activated by calling the activateProposal function below. However, the proposal should only be endorsed with these same votes once and should not be able to be activated if it could not satisify (totalEndorsementsForProposal[proposalId_] * 100) < VOTES.totalSupply() * ENDORSEMENT_THRESHOLD with these votes being used once.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180-L201

    function endorseProposal(uint256 proposalId_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (proposalId_ == 0) {
            revert CannotEndorseNullProposal();
        }

        Instruction[] memory instructions = INSTR.getInstructions(proposalId_);
        if (instructions.length == 0) {
            revert CannotEndorseInvalidProposal();
        }

        // undo any previous endorsement the user made on these instructions
        uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender];
        totalEndorsementsForProposal[proposalId_] -= previousEndorsement;

        // reapply user endorsements with most up-to-date votes
        userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
        totalEndorsementsForProposal[proposalId_] += userVotes;

        emit ProposalEndorsed(proposalId_, msg.sender, userVotes);
    }

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L205-L236

    function activateProposal(uint256 proposalId_) external {
        ProposalMetadata memory proposal = getProposalMetadata[proposalId_];

        if (msg.sender != proposal.submitter) {
            revert NotAuthorizedToActivateProposal();
        }

        if (block.timestamp > proposal.submissionTimestamp + ACTIVATION_DEADLINE) {
            revert SubmittedProposalHasExpired();
        }

        if (
            (totalEndorsementsForProposal[proposalId_] * 100) <
            VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
        ) {
            revert NotEnoughEndorsementsToActivateProposal();
        }

        if (proposalHasBeenActivated[proposalId_] == true) {
            revert ProposalAlreadyActivated();
        }

        if (block.timestamp < activeProposal.activationTimestamp + GRACE_PERIOD) {
            revert ActiveProposalNotExpired();
        }

        activeProposal = ActivatedProposal(proposalId_, block.timestamp);

        proposalHasBeenActivated[proposalId_] = true;

        emit ProposalActivated(proposalId_, block.timestamp);
    }

Proof of Concept

Please append the following test in src\test\policies\Governance.t.sol. This test will pass to demonstrate the described scenario.

    function testScenario_UserEndorsesAfterReceivingTransferredVotes() public {
        _submitProposal();

        vm.prank(voter2);
        governance.endorseProposal(1);

        // to simulate calling gOHM's transfer function by voter2 for sending votes to voter0, VOTES.transferFrom is called by governance here
        vm.prank(address(governance));
        VOTES.transferFrom(voter2, voter0, 200);

        // voter0 uses the votes previously owned by voter2 to endorse the proposal
        vm.prank(voter0);
        governance.endorseProposal(1);

        // the proposal is endorsed with 400 votes but only the 200 votes originally owned by voter2 are used
        assertEq(governance.userEndorsementsForProposal(1, voter0), 200);
        assertEq(governance.userEndorsementsForProposal(1, voter2), 200);
        assertEq(governance.totalEndorsementsForProposal(1), 400);

        // At this moment, the proposal can be activated successfully.
        // However, if it is endorsed with only 200 votes, it cannot satisfy ENDORSEMENT_THRESHOLD and cannot be activated.
        vm.expectEmit(true, true, true, true);
        emit ProposalActivated(1, block.timestamp);

        vm.prank(voter1);
        governance.activateProposal(1);
    }

Tools Used

VSCode

Recommended Mitigation Steps

When calling endorseProposal, the user’s votes can be locked by transferring these votes to the governance so the user cannot transfer these anymore to another user after the endorsement. An additional function can be added for reclaiming the endorsed votes back to the user and reducing the proposal’s endorsed votes accordingly before the proposal is activated. After the proposal is activated, the endorsed votes should be counted as the voted votes.

fullyallocated (Olympus) acknowledged and commented:

Taken from another issue:

This is true, and I appreciate the throughness of the explanation—it’s hard to adjust endorsements based on the user’s balance because there’s no events/callbacks in solidity. We plan to use a staking vault where tokens are transfer locked and there’s a warmup period + cooldown period to mitigate this issue.

[M-07] Endorsed votes by a user do not decrease after the user’s votes are revoked

Submitted by rbserver

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56
https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180-L201

The voter admin can call the following revokeVotesFrom function to revoke a user’s votes, which also decreases the total supply of the votes, after the user endorses a proposal through calling the endorseProposal function below. Because endorseProposal can be called multiple times, the user has the incentive to call it for endorsing the proposal again with the new votes minted by the issueVotesTo function. However, after the user’s votes are revoked, the user has no incentive to call endorseProposal again. Hence, the endorsed votes by the user for the proposal does not decrease after the user’s votes are revoked. When determining whether the proposal can be activated or not, its old endorsed votes, which is not decreased, are compared against the new total supply of the votes, which is decreased because of the revokeVotesFrom call. As a result, the proposal is unreliably more likely to satisfy the condition for being activated.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56

    function revokeVotesFrom(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
        // Revoke the votes in the VOTES module
        VOTES.burnFrom(wallet_, amount_);
    }

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180-L201

    function endorseProposal(uint256 proposalId_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (proposalId_ == 0) {
            revert CannotEndorseNullProposal();
        }

        Instruction[] memory instructions = INSTR.getInstructions(proposalId_);
        if (instructions.length == 0) {
            revert CannotEndorseInvalidProposal();
        }

        // undo any previous endorsement the user made on these instructions
        uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender];
        totalEndorsementsForProposal[proposalId_] -= previousEndorsement;

        // reapply user endorsements with most up-to-date votes
        userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
        totalEndorsementsForProposal[proposalId_] += userVotes;

        emit ProposalEndorsed(proposalId_, msg.sender, userVotes);
    }

Proof of Concept

Please append the following test in src\test\policies\Governance.t.sol. This test will pass to demonstrate the described scenario.

    function testScenario_EndorsedVotesDoNotDecreaseAfterVotesAreRevoked() public {
        _submitProposal();

        // voter3 endorse the proposal
        vm.prank(voter3);
        governance.endorseProposal(1);

        assertEq(governance.userEndorsementsForProposal(1, voter3), 300);
        assertEq(governance.totalEndorsementsForProposal(1), 300);

        // to simulate calling VoterRegistration.revokeVotesFrom that burns voter3's votes, VOTES.burnFrom is called by godmode here
        vm.prank(godmode);
        VOTES.burnFrom(voter3, 300);

        // at this moment, voter3 has 0 votes
        assertEq(VOTES.balanceOf(voter3), 0);

        // however, the proposal is still endorsed with voter3's previous votes
        assertEq(governance.userEndorsementsForProposal(1, voter3), 300);
        assertEq(governance.totalEndorsementsForProposal(1), 300);
    }

Tools Used

VSCode

Recommended Mitigation Steps

When revokeVotesFrom is called during the time for endorsement, the corresponding votes that are previously endorsed for a proposal and are now revoked should be removed from the proposal’s endorsed votes for the user. This ensures that the endorsed votes and the votes’ total supply after the revocation are in sync for the proposal.

fullyallocated (Olympus) acknowledged and commented:

This is true, and I appreciate the throughness of the explanation—it’s hard to adjust endorsements based on the user’s balance because there’s no events/callbacks in solidity. We plan to use a staking vault where tokens are transfer locked and there’s a warmup period + cooldown period to mitigate this issue.

[M-08] “TWAP” used is an observation-weighted-average-price, not a time-weighted one

Submitted by IllIllI

While users are incentivized to call the heartbeat, the incentive may be removed later, or it may be more profitable to use old prices, so users may not call the heartbeat during unfavorable prices, leading to the TWAP price being incorrect, and users getting the wrong price for their assets.

A similar case of an incomplete TWAP algorithm was found to be of Medium risk.

Proof of Concept

A TWAP is a Time-Weighted average price, but the algorithm below does not take into account the time between observations:

File: /src/modules/PRICE.sol   #1

134          // Calculate new moving average
135          if (currentPrice > earliestPrice) {
136              _movingAverage += (currentPrice - earliestPrice) / numObs;
137          } else {
138              _movingAverage -= (earliestPrice - currentPrice) / numObs;
139          }
140  
141          // Push new observation into storage and store timestamp taken at
142          observations[nextObsIndex] = currentPrice;
143          lastObservationTime = uint48(block.timestamp);
144:         nextObsIndex = (nextObsIndex + 1) % numObs;

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L134-L144

While the Heart policy enforces an upper bound on how frequently updates are added to the average, there is no guarantee that users call beat() in a timely manner:

File: /src/policies/Heart.sol   #2

92       function beat() external nonReentrant {
93           if (!active) revert Heart_BeatStopped();
94:          if (block.timestamp < lastBeat + frequency()) revert Heart_OutOfCycle();

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

The incentive may be set to too low an amount:

File: /src/policies/Heart.sol   #3

140      function setRewardTokenAndAmount(ERC20 token_, uint256 reward_)
141          external
142          onlyRole("heart_admin")
143      {
144          rewardToken = token_;
145          reward = reward_;
146          emit RewardUpdated(token_, reward_);
147:     }

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L140-L147

Or users may find it more profitable to skip a particular update, or front-run an unfavorable update, with a transaction that trades assets at the old price

Recommended Mitigation Steps

Always call an internal version of beat() that doesn’t revert, in functions that swap user assets. The code should also track the timestamps of when each beat() is called, and include the amount of time that has passed since the last beat, in the TWAP calculation

Oighty (Olympus) disagreed with severity and commented:

The referenced issue is a bit different than our use case since we will be using a much longer duration moving average. The goal is to get an approximate moving average over a certain period of time (e.g. 120 days) vs. an exact number since, as you say, the time of each observation cannot be guaranteed to be at a specific time. We believe that using a long duration with a sufficient number of observations will make this value close enough to the true value it is approximating, and prevents actors from manipulating the value by waiting to provide a specific value (1 out of ~360 obs doesn’t move the needle). The use of the “TWAP” term may be semantically inaccurate.

As for not guaranteeing that the update will be called or issues with several observations close to each other, see comments on #405 and #79.

The mitigations suggested do not seem to provide a solution that improves the system. Calling beat() on user actions would not have the observations roughly evenly spaced. Tracking timestamps is possible, but I don’t see how it improves the data.

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L205-L262

@Oighty - I think the warden is suggesting that the call to beat() in the user actions would do more to ensure that the “TWAP” stays up to date. If the call isn’t past the correct period, it would just return and make no change (costing some amount of gas, ofc).

I do think it may be worth considering, that way no user action can take place without the TWAP being as up to date as possible and no additional calls to the contract may be necessary if users are interacting with the contract frequently enough.

While this is related to #79 - I think the points raised here and the mitigation is sufficiently different to warrant this issue to stand alone.

Oighty (Olympus) commented:

That’s a fair point. One issue with calling beat on user actions, e.g. Operator.swap, is that it would update the wall price that the user is swapping at. Therefore, the call could fail due to the slippage check. This could be confusing behavior and may have unintended consequences of DOS’ing the system. Additionally, the gas cost of beat is highly variable (sometimes up to 600k gas when opening a bond market) and would cause some users to unexpectedly pay a lot more gas for a swap.

I’ll discuss with the team, but I don’t think the pros exceed the cons.

[M-09] `activateProposal()` need time delay

Submitted by __141345__, also found by 0x1f8b, Trust, V_B, and zzzitron

There is no time lock or delay when activating a proposal, the previous one could be replaced immediately. In vote() call, a user might want to vote for the previous proposal, but if the vote() call and the activateProposal() is very close or even in the same block, it is quite possible that the user actually voted for another proposal without much knowledge of. A worse case is some malicious user watching the mempool, and front run a big vote favor/against the activeProposal, effectively influence the voting result.

These situations are not what the governance intends to deliver, and might also affect the results of 2 proposals.

Proof of Concept

activateProposal() can take effect right away, replacing the activeProposal. And vote() does not specify which proposalId to vote for, but the activeProposal could be different from last second.

src/policies/Governance.sol

    function activateProposal(uint256 proposalId_) external {
        ProposalMetadata memory proposal = getProposalMetadata[proposalId_];

        if (msg.sender != proposal.submitter) {
            revert NotAuthorizedToActivateProposal();
        }

        if (block.timestamp > proposal.submissionTimestamp + ACTIVATION_DEADLINE) {
            revert SubmittedProposalHasExpired();
        }

        if (
            (totalEndorsementsForProposal[proposalId_] * 100) <
            VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
        ) {
            revert NotEnoughEndorsementsToActivateProposal();
        }

        if (proposalHasBeenActivated[proposalId_] == true) {
            revert ProposalAlreadyActivated();
        }

        if (block.timestamp < activeProposal.activationTimestamp + GRACE_PERIOD) {
            revert ActiveProposalNotExpired();
        }

        activeProposal = ActivatedProposal(proposalId_, block.timestamp);

        proposalHasBeenActivated[proposalId_] = true;

        emit ProposalActivated(proposalId_, block.timestamp);
    }

    function vote(bool for_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (activeProposal.proposalId == 0) {
            revert NoActiveProposalDetected();
        }

        if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
            revert UserAlreadyVoted();
        }

        if (for_) {
            yesVotesForProposal[activeProposal.proposalId] += userVotes;
        } else {
            noVotesForProposal[activeProposal.proposalId] += userVotes;
        }

        userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes;

        VOTES.transferFrom(msg.sender, address(this), userVotes);

        emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);
    }

Recommended Mitigation Steps

Add time delay when activating a proposal, so that users can be aware of that and vote for the current one within the time window.

fullyallocated (Olympus) disputed and commented:

This is a pretty unique edge case, I can acknowledge as QA.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L240-L262
https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L45-L48
https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56

I actually don’t think its that unique in the case of on chain voting. Imagine a scenario where a user submits a vote with low gas amounts and it is not mined for days later and then the active proposal has changed. I am not sure why the vote function wouldn’t take in the intended proposal ID.

I am going to leave as medium severity as I do think this impacts the intended functionality of the protocol, but am willing to hear more from the sponsor on why they disagree.

[M-10] Voted votes cannot change after the user is issued new votes or the user’s old votes are revoked during voting

Submitted by rbserver, also found by __141345__, cccz, csanuragjain, GalloDaSballo, Guardian, Lambda, m9800, and zzzitron

A user can call the following vote function to vote for a proposal. During voting, the voter admin can still call the issueVotesTo and revokeVotesFrom functions below to issue new votes or revoke old votes for the user, which also changes the votes’ total supply during the overall voting. Because each user can only call vote once for a proposal due to the userVotesForProposal[activeProposal.proposalId][msg.sender] > 0 conditional check, the old voted votes, resulted from the vote call by the user, will be used to compare against the new total supply of the votes, resulted from the issueVotesTo and revokeVotesFrom calls during the overall voting, when determining whether the proposal can be executed or not. Because of this inconsistency, the result on whether the proposal can be executed might not be reliable.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L240-L262

    function vote(bool for_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (activeProposal.proposalId == 0) {
            revert NoActiveProposalDetected();
        }

        if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
            revert UserAlreadyVoted();
        }

        if (for_) {
            yesVotesForProposal[activeProposal.proposalId] += userVotes;
        } else {
            noVotesForProposal[activeProposal.proposalId] += userVotes;
        }

        userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes;

        VOTES.transferFrom(msg.sender, address(this), userVotes);

        emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);
    }

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L45-L48

    function issueVotesTo(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
        // Issue the votes in the VOTES module
        VOTES.mintTo(wallet_, amount_);
    }

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56

    function revokeVotesFrom(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
        // Revoke the votes in the VOTES module
        VOTES.burnFrom(wallet_, amount_);
    }

Proof of Concept

Please add the following code in src\test\policies\Governance.t.sol.

First, please add the following code for stdError.

import {Test, stdError} from "forge-std/Test.sol";    // @audit import stdError for testing purpose

Then, please append the following tests. These tests will pass to demonstrate the described scenarios.

    function testScenario_UserCannotVoteAgainWithNewlyMintedVotes() public {
        _createActiveProposal();

        // voter3 votes for the proposal
        vm.prank(voter3);
        governance.vote(true);

        assertEq(governance.yesVotesForProposal(1), 300);
        assertEq(governance.noVotesForProposal(1), 0);

        assertEq(governance.userVotesForProposal(1, voter3), 300);
        assertEq(VOTES.balanceOf(voter3), 0);
        assertEq(VOTES.balanceOf(address(governance)), 300);

        // to simulate calling VoterRegistration.issueVotesTo that mints votes to voter3, VOTES.mintTo is called by godmode here
        vm.prank(godmode);
        VOTES.mintTo(voter3, 500);
        assertEq(VOTES.balanceOf(voter3), 500);

        // calling vote function again by voter3 reverts, which means that voter3 cannot additionally vote with the 500 newly minted votes
        vm.expectRevert(UserAlreadyVoted.selector);
        vm.prank(voter3);
        governance.vote(true);
    }

    function testScenario_RevokeVotesAfterUserFinishsOwnVoting() public {
        _createActiveProposal();

        // voter3 votes for the proposal
        vm.prank(voter3);
        governance.vote(true);

        assertEq(governance.yesVotesForProposal(1), 300);
        assertEq(governance.noVotesForProposal(1), 0);

        assertEq(governance.userVotesForProposal(1, voter3), 300);
        assertEq(VOTES.balanceOf(voter3), 0);
        assertEq(VOTES.balanceOf(address(governance)), 300);

        // To simulate calling VoterRegistration.revokeVotesFrom that burns voter3's votes, VOTES.burnFrom is called by godmode here.
        // However, calling VOTES.burnFrom will revert due to arithmetic underflow.
        vm.prank(godmode);
        vm.expectRevert(stdError.arithmeticError);
        VOTES.burnFrom(voter3, 300);

        // the proposal is still voted with voter3's previous votes afterwards
        assertEq(governance.userVotesForProposal(1, voter3), 300);
        assertEq(VOTES.balanceOf(voter3), 0);
        assertEq(VOTES.balanceOf(address(governance)), 300);
    }

Tools Used

VSCode

Recommended Mitigation Steps

When issueVotesTo and revokeVotesFrom are called during voting, the corresponding votes need to be added to or removed from the proposal’s voted votes for the user. Alternatively, issueVotesTo and revokeVotesFrom can be disabled when an active proposal exists.

fullyallocated (Olympus) confirmed and commented:

This is the best written answer.

Originally votes were locked so that users cannot constantly change their vote to manipulate the outcome but the warden makes a good point about how the quorum thresholds can be changed and the affects on how consensus is calculated.

[M-11] OlympusGovernance: Users can prevent their votes from being revoked

Submitted by cccz, also found by zzzitron

In the VoterRegistration contract, voter_admin can call the revokeVotesFrom function to revoke the user’s votes.

    function revokeVotesFrom(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
        // Revoke the votes in the VOTES module
        VOTES.burnFrom(wallet_, amount_);
    }

But there is a way for users to prevent their votes from being revoked by voteradmin.
In the OlympusGovernance contract, the user can call the vote function to vote for the activeProposal, and then call the reclaimVotes function to reclaim his votes.
When the vote function is called, VOTES are sent to the OlympusGovernance contract and recorded using the userVotesForProposal variable. When the reclaimVotes function is called, the VOTES recorded in the userVotesForProposal variable are sent back to the user.
This means that the user can store his VOTES tokens in userVotesForProposal.
The revokeVotesFrom function cannot revoke the VOTES tokens recorded in userVotesForProposal and the reclaimVotes function can only be called by the user himself.
If the user calls the reclaimVotes function and vote function in one transaction, then his VOTES token balance will always be 0 (thus avoiding revocation of votes by voteradmin) and he will be able to vote.

Proof of Concept

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L240-L262
https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L295-L313
https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/VoterRegistration.sol#L53-L56

Recommended Mitigation Steps

Consider allowing to call the reclaimVotes function to reclaim any user’s vote, thus avoiding the user storing his VOTES tokens in userVotesForProposal

    function reclaimVotes(uint256 proposalId_, address user_) external {
        uint256 userVotes = userVotesForProposal[proposalId_][user_];

        if (userVotes == 0) {
            revert CannotReclaimZeroVotes();
        }

        if (proposalId_ == activeProposal.proposalId) {
            revert CannotReclaimTokensForActiveVote();
        }

        if (tokenClaimsForProposal[proposalId_][user_] == true) {
            revert VotingTokensAlreadyReclaimed();
        }

        tokenClaimsForProposal[proposalId_][user_] = true;

        VOTES.transferFrom(address(this), user_, userVotes);
    }

fullyallocated (Olympus) acknowledged and commented:

This is true, we don’t expect to use the voter admin in production, just to issue votes during internal testing period.

0xean (judge) decreased severity to Medium and commented:

Downgrading to M severity as this does not lead to direct loss of user funds, but does highlight an issue with current contracts.

cccz (warden) commented:

Consider the following scenarios. There are currently three users, A, B and C, in the system.

voter_admin minted 100 VOTEs for each of these three users

After a period of time, due to system upgrade or other reasons, the VOTEs of the users need to be revoked.

voter_admin revokes the VOTEs of users A and B respectively, but user C uses this vulnerability to prevent his VOTE from being revoked.

At this time, user C has all the VOTEs, and he can execute any proposal.

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/TreasuryCustodian.sol#L53-L67

Okay, at this point I still believe a Medium issue, voter_admin as a mitigation could reissue votes to User A and B. Additionally User C will eventually have to reclaim these votes in order to vote on the next proposal. I am going to stick with Medium on this one. Appreciate the additional clarity.

cccz (warden) commented:

@0xean - You are right, thanks for your attention.

[M-12] Griefing/DOS of withdrawals by EOAs from treasury (TRSRY) possible

Submitted by minhtrng

Any withdrawals from the treasury by an approved EOA can be denied by a malicious actor that watches the mempool.

Proof of Concept

The function TreasuryCustodian.revokePolicyApprovals() doesnt provide sufficient checks for its intended purpose of “revoking a deactivated policy’s approvals”. As can be seen by the TODO labels, the issue has already been acknowledged by the team (regardless it is still an issue present in an in-scope contract). The only check performed is trying to call the isActive()-function on an address and interpret the returned value as boolean. Attempting to call this function on an EOA will not fail and return 0 (=false). Hence the condition to revert is not fulfilled and the amounts approved to withdraw will be set to 0.

Tools Used

IDE (Remix, VSCode)

Recommended Mitigation Steps

A partial but insufficient fix would be to check if the address passed to the function contains code and hence is not an EOA. A better approach might be to add a mapping(address => bool) of all addresses that have been active policies some time in the past to the kernel, something like this:

As a public variable in Kernel.sol mapping(address => bool) public isRegisteredPolicy;

in Kernel.activatePolicy(): isRegisteredPolicy[address(policy_)] ) = true;

and finally in TreasuryCustodian.revokePolicyApprovals(): if(!kernel.isRegisteredPolicy(policy_) revert NotARegisteredPolicy

https://github.com/code-423n4/2022-08-olympus/blob/main/src/Kernel.sol#L325

TODOs are outdated, I forgot to clear them ;(. But the points are taken. Code will be adjusted, but probably not the way from the recommendation. Instead will gate the function behind custodian role.

[M-13] Missing checks in `Kernel._deactivatePolicy`

Submitted by enckrish

There are no checks to ascertain that the policy being removed is registered in the Kernel. Trying to remove a non-registered results in the policy registered at 0th index of activePolicies being removed.

Proof of Concept

Recommended Mitigation Steps

Adding require(activePolicies[idx] == policy_, "Unregistered policy"); will prevent this, where idx = getPolicyIndex[policy_].

NOTE: The issue is less likely to happen as this is handled solely by the executor, but having safeguards in the contract is always better than relying on an external factor.

ind-igo (Olympus) confirmed, but disagreed with severity and commented:

Confirmed. Should be lower risk or a QA issue.

@ind-igo - can you comment on why you think it should be QA vs Medium?

— Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

I would expect this to impact the functionality of the protocol.

Oighty (Olympus) commented:

This one seems on the fence to me. While accidentally unregistering a policy likely would affect the functionality of the protocol, it requires the executor to make a mistake. If the mistake is made, it’s easily remedied by re-registering the policy.

fullyallocated (Olympus) acknowledged

That makes sense, but there would be some amount of down time when this occurred. I think Medium is correct for this issue.

[M-14] The governance system can be held hostage by a malicious user

Submitted by d3e4, also found by Aymen0909 and pedroais

With only ENDORSEMENT_THRESHOLD% (currently 20%) voting power, a malicious user can prevent any other proposal from being activated. While ENDORSEMENT_THRESHOLD is currently fairly high, it seems not higher than that it might not be used to hold the system hostage to extract far more funds.

Proof of Concept

Submit a dummy proposal, endorse it and then activate it. Now, no other proposal can be activated for a GRACE_PERIOD. When this time period is over, this procedure may be repeated, either immediately or just before any other proposal activation by front-running.

Recommended Mitigation Steps

Making sure ENDORSEMENT_THRESHOLD is at least 50% seems discouraging enough. Other more creative solutions should be possible. One might be to let the most endorsed proposal be activated, or restricting who can activate a proposal; anything that at least temporarily liberates the governance system so that the attacker is dissuaded from investing in this attack method.

[M-15] Heart will stop if all rewards are swept

Submitted by GalloDaSballo, also found by cccz, itsmeSTYJ, and PwnPatrol

Rewards for Heart beat are sent via _issueReward

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L110-L115

    function _issueReward(address to_) internal {
        rewardToken.safeTransfer(to_, reward);
        emit RewardIssued(to_, reward);
    }

The function doesn’t check for available tokens e.g.
min(reward, rewardToken.balanceOf(address(this)));

In case of calling withdrawUnspentRewards

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L149-L152

    /// @inheritdoc IHeart
    function withdrawUnspentRewards(ERC20 token_) external onlyRole("heart_admin") {
        token_.safeTransfer(msg.sender, token_.balanceOf(address(this)));
    }

Because the function withdraws the entire amount, the heart will stop until a caller incentive is deposited again.

While a profitable searches will stop calling the Heart without an incentive, allowing the heart to beat when no rewards are available is preferable to having it self-DOS until a DAO aligned caller donates rewardToken or the DAO deals with the lack of tokens.

Recommended Mitigation Steps

Add a check for available tokens min(reward, rewardToken.balanceOf(address(this)));

Oighty (Olympus) disagreed with severity and commented:

Agree based on the anti-DOS characteristics of using a min operation.

[M-16] Inconsistant parameter requirements between `constructor()` and `Set() functions` in `RANGE.sol` and `Operator.sol`.

Submitted by hansfriese, also found by datapunk and itsmeSTYJ

Inconsistant parameter requirements between constructor and Set() functions in RANGE.sol and Operator.sol.

The contracts might work unexpectedly when the params are set improperly using constructor().

Proof of Concept

In RANGE.sol, setSpreads() and setThresholdFactor() has some requirements but constructor() doesn’t check at all.

    File: 2022-08-olympus\src\modules\RANGE.sol
    242:     function setSpreads(uint256 cushionSpread_, uint256 wallSpread_) external permissioned {
    243:         // Confirm spreads are within allowed values
    244:         if (
    245:             wallSpread_ > 10000 ||
    246:             wallSpread_ < 100 ||
    247:             cushionSpread_ > 10000 ||
    248:             cushionSpread_ < 100 ||
    249:             cushionSpread_ > wallSpread_
    250:         ) revert RANGE_InvalidParams();
    251: 
    252:         // Set spreads
    253:         _range.wall.spread = wallSpread_;
    254:         _range.cushion.spread = cushionSpread_;
    255: 
    256:         emit SpreadsChanged(wallSpread_, cushionSpread_);
    257:     }

    File: 2022-08-olympus\src\modules\RANGE.sol
    263:     function setThresholdFactor(uint256 thresholdFactor_) external permissioned {
    264:         if (thresholdFactor_ > 10000 || thresholdFactor_ < 100) revert RANGE_InvalidParams();
    265:         thresholdFactor = thresholdFactor_;
    266: 
    267:         emit ThresholdFactorChanged(thresholdFactor_);
    268:     }
    269:

In Operator.sol, setCushionFactor() checks the requirement but constructor() doesn’t check it.

    File: 2022-08-olympus\src\policies\Operator.sol
    516:     function setCushionFactor(uint32 cushionFactor_) external onlyRole("operator_policy") {
    517:         /// Confirm factor is within allowed values
    518:         if (cushionFactor_ > 10000 || cushionFactor_ < 100) revert Operator_InvalidParams();
    519: 
    520:         /// Set factor
    521:         _config.cushionFactor = cushionFactor_;
    522: 
    523:         emit CushionFactorChanged(cushionFactor_);
    524:     }
    525:

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend adding same validation for the parameters between constructor() and Set() functions.

Agree that the constructor should validate these parameters, but it is only an issue if configured improperly.

fullyallocated (Olympus) disputed and commented:

While I am typically weary of marking input validations as medium severity, I do think in this case it’s warranted as it directly leads to malfunctions at the protocol level and it seems that the sponsors thought it important enough to add the checks elsewhere. Hard call, but will award it at medium severity.

[M-17] No Cap on Amount of VOTES means the `voter_admin` can get any proposal to pass

Submitted by GalloDaSballo, also found by 0xNazgul, IllIllI, and rbserver

Because VOTES can be minted by voter_admin, and there is no cap on totalSupply, the voter_admin has the privileged ability to mint as many VOTES as they want in order to get any proposal to pass or veto it.

Proof of Concept - Veto

Be voter_admin
Mint XYZ tokens
totalSupply is now higher so any proposal can be vetoed per this line

Proof of Concept - Approve

Be voter_admin
Mint XYZ tokens, where XYZ allows the netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD check to pass
Mint VOTES to self
Vote
Proposal has passed

Recommended Mitigation Steps

Add a total supply cap to VOTES.

This is possible but will not happen in a production environment because we’re using this for internal testing.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L165-L171

Given the scope of the contracts the wardens were asked to review, I think this issue is valid. While I understand that the voter_admin is trusted, I don’t think users expect the admin to be able to bypass any votes results in this manner.

[M-18] Inconsistency in staleness checks between OHM and reserve token oracles

Submitted by okkothejawa, also found by datapunk, reassor, and Trust

Price oracle may fail and revert due to the inconsistency in the staleness checks.

Proof of Concept

In the getCurrentPrice() of PRICE.sol, Chainlink oracles are used to get the price of OHM against a reserve token, and a staleness check is used to make sure the price oracles are reporting fresh data. Yet the freshness requirements are inconsistent, for OHM, updatedAt should be lower than current timestamp minus three times the observation frequency, while for the reserve price, it is required that updatedAt should be lower than current timestamp minus the observation frequency. Our understanding is that that frequency is multiplied by 3 so that there can be some meaningful room where price data is accepted, as the time frame of only observation frequency (multiplied by 1) may not be enough for the oracle to realistically update its data. (In other words, the frequency of new price information might be lower than the observation frequency, which is probably the case as third multiple is used for the OHM price). If this is the case, this inconsistency may lead to the getCurrentPrice() reverting as while third multiple of the observation frequency might give enough space for the first oracle, second oracle’s first multiple of frequency time frame might not be enough and it couldn’t pass the staleness check due to unrealistic expectation of freshness.

Tools Used

Manual review, talking with devs

Recommended Mitigation Steps

Change the line 171 to

            if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))

like line 165.

This should indeed be the same. We will update to fix.

[M-19] TRSRY: reenter from `OlympusTreasury::repayLoan` to `Operator::swap`

Submitted by zzzitron

One can repay loan to the treasury with the value from the Operator::swap.

Condition:

the reserve token in Operator has hook for sender (like ERC777)
the debt is the same token as reserve

Proof of Concept

The below code snippet shows a part of proof of concept for reentrancy attack, which is based on src/test/policies/Operator.t.sol. The full test code can be found here, and git diff from the Operator.t.sol.

Let’s say that the reserve token implements ERC777 with the hook for the sender (see weird erc20). If the attacker can take debt of the reserve currency for the attack contract Reenterer, the contract can call OlympusTreasury::repayLoan and in the middle of repay call Operator::swap function. The swap function will modify the reserve token balance of treasury and the amount the attacker swapped will be also be used for the repayLoan.

In the below example, the attacker has debt of 1e18, and repays 1e17. But since the swap function is called in the repayLoan, the debt is reduced 1e17 more then it should. And the swap happened as expected so the attack has the corresponding ohm token.

/// Mock to simulate the senders hook
/// for simplicity omitted the certain aspects like ERC1820 registry and etc.
contract MockERC777 is MockERC20 {
    constructor () MockERC20("ERC777", "777", 18) {}

    function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
        _callTokenToSend(from, to, amount);
        return super.transferFrom(from, to, amount);
        // _callTokenReceived(from, to, amount);
    }

    // simplified implementation for ERC777
    function _callTokenToSend(address from, address to, uint256 amount) private {
      if (from != address(0)) {
        IERC777Sender(from).tokensToSend(from, to, amount);
      }
    }
}

interface IERC777Sender {
  function tokensToSend(address from, address to, uint256 amount) external;
}

/// Concept for an attack contract
contract Reenterer is IERC777Sender {
  ERC20 public token;
  Operator public operator;
  bool public entered;

  constructor(address token_, Operator op_) {
    token = ERC20(token_);
    operator = op_;
  }

  function tokensToSend(address from, address to, uint256 amount) external override {
    if (!entered) {
    // call swap from reenter
    // which will manipulate the balance of treasury
      entered = true;
      operator.swap(token, 1e17, 0);
    }
  }
  
  function attack(OlympusTreasury treasury) public {
    // approve to the treasury
    token.approve(address(treasury), 1e18);
    token.approve(address(operator), 100* 1e18);

    // repayDebt of 1e17
    treasury.repayLoan(token, 1e17);
  }
}

/// the test
    function test_poc__reenter() public {
        vm.prank(guardian);
        operator.initialize();

      reserve.mint(address(reenterer), 1e18);
      assertEq(treasury.reserveDebt(reserve, address(reenterer)), 1e18);
      // start repayLoan
      reenterer.attack(treasury);
      // it should be 9 * 1e17 but it is 8 * 1e17
      assertEq(treasury.reserveDebt(reserve, address(reenterer)), 8*1e17);
    }

Cause

The repayLoan, in the line 110 below, calls the safeTransferFrom. The balance before and after are compared to determine how much of debt is paid. So, if the safeTranferFrom can modify the balance, the attacker can profit from it.

// OlympusTreasury::repayLoan

105     function repayLoan(ERC20 token_, uint256 amount_) external nonReentrant {
106         if (reserveDebt[token_][msg.sender] == 0) revert TRSRY_NoDebtOutstanding();
107
108         // Deposit from caller first (to handle nonstandard token transfers)
109         uint256 prevBalance = token_.balanceOf(address(this));
110         token_.safeTransferFrom(msg.sender, address(this), amount_);
111
112         uint256 received = token_.balanceOf(address(this)) - prevBalance;

In the swap function, if the amount in token is reserve, the payment token to buy ohm will be paid to the treasury. It gives to an opportunity to modify the balance.

// Operator::swap

329             /// Transfer reserves to treasury
330             reserve.safeTransferFrom(msg.sender, address(TRSRY), amountIn_);

Although both of Operator::swap and OlympusTreasury::repayLoan have nonReentrant modifier, it does not prevent as they are two different contracts.

Tools Used

Foundry

Recommended Mitigation Steps

The deposit logic in the OlympusTreasury::repayLoan was trying to handle nonstandard tokens, such as fee-on-transfer. But by doing so introduced an attack vector for tokens with ERC777. If the reserve token should be decided in the governance, it should be clarified, which token standards can be used safely.

Good report, although very low risk as the preconditions are extremely unlikely. Will take into account the suggestion by adding a comment to the function. Thank you.

Oighty (Olympus) disagreed with severity and commented:

I would probably downgrade to QA, but the warden does a good job of proving the point out with examples. Will leave as Medium.

[M-20] Operator: if WallSpread is 10000, `operate` and `beat` will revert and price information cannot be updated anymore

Submitted by zzzitron

The beat cannot be called anymore and price information will not be updated

Condition:

the wallspread is set to 10000 (100%)
lower wall is active (range.low.active==true)
the price falls into the lower cushion (currentPrice < range.cushion.low.price && currentPrice > range.wall.low.price), therefore activates the lower bond market

Proof of Concept

The below proof of concept demonstrates that the operate will revert with 100% wallspread. The full test code can be found here as well as the diff from Operator.t.sol.

In the test, the wallspread was set to 10000, which is 100% (line 51). The price was set so that the lower market should be deployed (line 59). In the market deployment logic (Operator::_activate) will revert due to division by zero, and operate will fail.

Once this condition is met, the operate cannot be called and Heart::beat cannot be called as well, since the Heart::beat is calling Operator::opearate under the hood. As the result the price can never be updated. But other codes who uses the price information will not know that the price information is stale. If the upper wall is active and still have the capacity, one can swap from the upper wall using the stale information, which might cause some loss of funds.

    function test_poc__lowCushionDeployWithWallspread10000Reverts() public {
        /// Initialize operator
        vm.prank(guardian);
        operator.initialize();

        /// if the wallspread is 10000 the operate might revert
        vm.prank(policy);
        operator.setSpreads(7000, 10000);

        /// Confirm that the cushion is not deployed
        assertTrue(!auctioneer.isLive(range.market(true)));

        /// Set price on mock oracle into the lower cushion
        /// given the lower wallspread is 10000
        /// when the lower market should be deployed the operate reverts
        price.setLastPrice(20 * 1e18);

        /// Trigger the operate function manually
        /// The operate will revert Error with "Division or modulo by 0"
        /// But I could not figure out to catch with `expectRevert`
        /// so just commenting out the assert
        // vm.prank(guardian);
        // /// vm.expectRevert(bytes("Division or module by 0"));   // this cannot catch the revert...
        // operator.operate();
    }

Cause

The main cause is that the RANGE::setSpreads function fails to check for wallSpread_ == 10000. If the setter does not allow the wallSpread to be 100%, the price of the lower wall will not go to zero.

// modules/RANGE.sol


242     function setSpreads(uint256 cushionSpread_, uint256 wallSpread_) external permissioned {
243         // Confirm spreads are within allowed values
244         if (
245             wallSpread_ > 10000 ||
246             wallSpread_ < 100 ||
247             cushionSpread_ > 10000 ||
248             cushionSpread_ < 100 ||
249             cushionSpread_ > wallSpread_
250         ) revert RANGE_InvalidParams();

In the RANGE::updatePrices, the price of lower wall will be zero if the wallSpread is 100%. If the price of lower wall is zero, the Operator::_activate will fail for the lower cushion.

// policies/Operator.sol::_activate(bool high_)
// when high_ is false
421             uint256 invWallPrice = 10**(oracleDecimals * 2) / range.wall.low.price;

// modules/RANGE.sol::updatePrices
164         _range.wall.low.price = (movingAverage_ * (FACTOR_SCALE - wallSpread)) / FACTOR_SCALE;

Tools Used

Foundry

Recommended Mitigation Steps

Mitigation suggestion: line 245. Forbid wallSpread to be 100%.

// modules/RANGE.sol


242     function setSpreads(uint256 cushionSpread_, uint256 wallSpread_) external permissioned {
243         // Confirm spreads are within allowed values
244         if (
-245            wallSpread_ > 10000 ||
+               wallSpread_ >= 10000 ||
246             wallSpread_ < 100 ||
247             cushionSpread_ > 10000 ||
248             cushionSpread_ < 100 ||
249             cushionSpread_ > wallSpread_
250         ) revert RANGE_InvalidParams();

This is indeed an edge case and we will update the value checks for the spread values to exclude 10000. However, from a practical perspective, this is very unlikely to happen. If the goal is to set the lower wall to 0, then the system would just be disabled.

Given the warden does fully demonstrate the issue, I am going to award as Medium with the understanding that this is an extreme edge case.

[M-21] OlympusGovernance - active proposal does not expire

Submitted by reassor

Contract OlympusGovernance allows controlling protocol through on-chain governing. The issue is that once proposal is active it does not expire, which means that until the new proposal will be selected, anyone can vote on existing one and potentially execute it when it might cause harm to the protocol.

Scenario:

New proposal has been submited, endorsed and activated.
Users vote, but the quroum is not being achieved.
The proposal is active until new one is getting submitted.
6 months elapses and the current active proposal might cause serious harm to the protocol (since it was created long time ago).
Malicious actor votes and executes proposal causing harm to the protocol.

Proof of Concept

Governance.sol:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L265-L289

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add expiration for the active proposal for example 2 weeks. After that time it should be possible to reject proposal and users should be able to reclaim VOTES tokens.

fullyallocated (Olympus) disputed

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/TRSRY.sol#L60

I believe the warden is simply stating that an active proposal stays active if not replaced. There is not expiration of a proposal once it becomes active, so theoretically if the governance process is inactive a very stale proposal could get executed.

[M-22] Low market bonds/swaps not working after loan is taken from treasury

Submitted by immeas

I am bordering between this being medium and low, but since this is, granted under very unlikely circumstances, hindering intended transfers to work I am submitting it as medium. That said, I don’t think this scenario is very likely since it requires a trusted contract not part of initial release(? no contract in repo used a loan) to take a large loan from TRSRY.

Proof of Concept

This will cause test to fail on TRANSFER_FAILED due to TRSRY not having the tokens to transfer but getReserveBalance says it has, since capacity is determined based on non-existing tokens.

diff --git a/src/test/policies/Operator.t.sol b/src/test/policies/Operator.t.sol
index e09aec1..5c1e95f 100644
--- a/src/test/policies/Operator.t.sol
+++ b/src/test/policies/Operator.t.sol
@@ -26,6 +26,8 @@ import {OlympusMinter, OHM} from "modules/MINTR.sol";
 import {Operator} from "policies/Operator.sol";
 import {BondCallback} from "policies/BondCallback.sol";
 
+import {ModuleTestFixtureGenerator} from "test/lib/ModuleTestFixtureGenerator.sol";
+
 contract MockOhm is ERC20 {
     constructor(
         string memory _name,
@@ -45,6 +47,7 @@ contract MockOhm is ERC20 {
 // solhint-disable-next-line max-states-count
 contract OperatorTest is Test {
     using FullMath for uint256;
+    using ModuleTestFixtureGenerator for OlympusTreasury;
 
     UserFactory public userCreator;
     address internal alice;
@@ -53,6 +56,9 @@ contract OperatorTest is Test {
     address internal policy;
     address internal heart;
 
+    address public debtor;
+    address public godmode; 
+
     RolesAuthority internal auth;
     BondAggregator internal aggregator;
     BondFixedTermTeller internal teller;
@@ -187,6 +193,18 @@ contract OperatorTest is Test {
 
         reserve.mint(address(treasury), testReserve * 100);
 
+        debtor = treasury.generateFunctionFixture(treasury.getLoan.selector);
+        godmode = treasury.generateGodmodeFixture(type(OlympusTreasury).name);
+        
+        kernel.executeAction(Actions.ActivatePolicy, godmode);
+        kernel.executeAction(Actions.ActivatePolicy, debtor);
+        
+        vm.prank(godmode);
+        treasury.setApprovalFor(debtor, reserve, testReserve * 100);
+
+        vm.prank(debtor);
+        treasury.getLoan(reserve,testReserve*100);
+
         // Approve the operator and bond teller for the tokens to swap
         vm.prank(alice);
         ohm.approve(address(operator), testOhm * 20);

Same is applicable for low market bonds since they are created based on the same capacity.

Tools Used

vs code + tests

Recommended Mitigation Steps

Determine capacity from actual tokens held by treasury.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/TRSRY.sol#L108-L112

Acknowledged. Will add a reserve requirement check inside the TRSRY’s debt functions, which we can expand with a policy to rebalance if out of balance on a heartbeat.

[M-23] Treasury module is vulnerable to cross-contract reentrancy

Submitted by Czar102

An attacker can pay back their loan to the treasury module with protocol-owned tokens. This will cause their loan to decrease despite the protocol won’t be given funds for it.

Proof of Concept

The code first measures the number of tokens in the treasury, then transfers an amount to the contract and checks the change it caused. This is put behind a nonReentrant modifier so that one can’t use the same balance change to pay back multiple parts of (potentially) multiple loans.

The problem arises when the treasury doesn’t only claim tokens from paying back loans, but also claims protocol revenue. Since, an attacker can gain execution in the moment the funds are pulled to the treasury to trigger any function that grants treasury this type of tokens (collects protocol revenue). The contract will count these tokens as paying back one’s loan since this happened between balance measurements.

Recommended Mitigation Steps

Add a function used to pull a token to the contract and mark it nonReentrant. Any transfer of tokens to the treasury should be done through that function.

I am confused by this submission. Need more information.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L161
https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L170

Spoke with Czar, solution for minimal change is adding received = min(received, amount_);. Confirming issue.

[M-24] [NAZ-M1] Chainlink’s `latestRoundData` Might Return Stale Results

Submitted by 0xNazgul, also found by __141345__, 0x1f8b, ak1, brgltd, cccz, csanuragjain, Dravee, Guardian, hyh, IllIllI, itsmeSTYJ, Jujic, Lambda, pashov, peachtea, rbserver, reassor, Sm4rty, TomJ, and zzzitron

Across these contracts, you are using Chainlink’s latestRoundData API, but there is only a check on updatedAt. This could lead to stale prices according to the Chainlink documentation:

The result of latestRoundData API will be used across various functions, therefore, a stale price from Chainlink can lead to loss of funds to end-users.

Recommended Mitigation Steps

Consider adding the missing checks for stale data.

For example:

(uint80 roundID ,answer,, uint256 timestamp, uint80 answeredInRound) = AggregatorV3Interface(chainLinkAggregatorMap[underlying]).latestRoundData();

require(answer > 0, "Chainlink price <= 0"); 
require(answeredInRound >= roundID, "Stale price");
require(timestamp != 0, "Round not complete");

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L134-L139

Agree. We’ll add the additional checks.

[M-25] Moving average precision is lost

Submitted by hyh, also found by CertoraInc, d3e4, and rbserver

Now the precision is lost in moving average calculations as the difference is calculated separately and added each time, while it typically can be small enough to lose precision in the division involved.

For example, 10000 moves of 990 size, numObservations = 1000. This will yield 0 on each update, while must yield 9900 increase in the moving average.

Proof of Concept

Moving average is calculated with the addition of the difference:

        // Calculate new moving average
        if (currentPrice > earliestPrice) {
            _movingAverage += (currentPrice - earliestPrice) / numObs;
        } else {
            _movingAverage -= (earliestPrice - currentPrice) / numObs;
        }

/ numObs can lose precision as currentPrice - earliestPrice is usually small.

It is returned on request as is:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L189-L193

    /// @notice Get the moving average of OHM in the Reserve asset over the defined window (see movingAverageDuration and observationFrequency).
    function getMovingAverage() external view returns (uint256) {
        if (!initialized) revert Price_NotInitialized();
        return _movingAverage;
    }

Recommended Mitigation Steps

Consider storing the cumulative sum, while returning sum / numObs on request:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L189-L193

    /// @notice Get the moving average of OHM in the Reserve asset over the defined window (see movingAverageDuration and observationFrequency).
    function getMovingAverage() external view returns (uint256) {
        if (!initialized) revert Price_NotInitialized();
-       return _movingAverage;
+       return _movingAverage / numObservations;
    }

Oighty (Olympus) disagreed with severity and commented:

Keeping track of the observations as a sum and then dividing is a good suggestion. The price values have 18 decimals and the max discrepancy introduced is very small (10**-15) with expected parameter ranges. The potential risk to the protocol seems low though.

hyh (warden) commented:

Please notice that discrepancy here is unbounded, i.e. the logic itself does not have any max discrepancy, the divergence between fact and recorded value can pile up over time without a limit.

If you do imply that markets should behave in some way that minuses be matched with pluses, then I must say that they really shouldn’t.

Oighty (Olympus) confirmed

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L363-L469

Debating between QA and Medium on this one. I am going to award it as medium because there is a potential to leak some value due to this imprecision compounding over time.

[M-26] Cushion bond markets are opened at wall price rather than current price

Submitted by 0x52

Incorrect initial bond market price.

Proof of Concept

uint256 initialPrice = range.wall.high.price.mulDiv(bondScale, oracleScale);

uint256 initialPrice = invWallPrice.mulDiv(bondScale, oracleScale);

In the above lines the initial prices are set to the wall price rather than the current price as indicated in documentation.

Recommended Mitigation Steps

Initial price should be updated to open bond market at current price rather than wall price.

Oighty (Olympus) disagreed with severity and commented:

This is more of a design decision than a bug. However, we did make this change in the code prior to the audit (it didn’t get reflected in the repo). @ind-igo not sure how you want to handle.

https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/INSTR.sol#L61

Going to award as Medium assuming no additional input from sponsor on the topic.

Oighty (Olympus) commented:

It does deviate from the spec so I guess that’s appropriate. The system actually would work as-is, but would be less responsive to price movements into the cushions since the bond market would have to decay (which requires waiting) to reach the current market price vs. instantly providing a buy/sell at the current price.

[M-27] Unexecutable proposals when `Actions.MigrateKernel` is not last instruction

Submitted by Lambda

In INSTR.sol, it is correctly checked that a ChangeExecutor instruction only occurs at the last position to avoid situations where the other instructions are deemed as invalid.
However, the same problem can occur for MigrateKernel. For instance, let’s say we have a MigrateKernel followed by a DeactivatePolicy action. The MigrateKernel action will change the value of kernel within the policy. The DeactivatePolicy action tries to call setActiveStatus on the policy. However, this has a onlyKernel modifier and the call will therefore fail when it is done after the value of kernel was changed.

Recommended Mitigation Steps

Perform the same check for MigrateKernel.

fullyallocated (Olympus) confirmed and commented:

Thank you; good catch

[M-28] Activating same Policy multiple times in Kernel possible

Submitted by Lambda, also found by enckrish

https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/Kernel.sol#L296

To check that an already active policy is not added a second time, isActive() is called on the policy. However, policy could be a malicious contract that always returns false for isActive(). In such a scenario, it would be possible to activate the policy multiple times for the same Kernel. This would break uniqueness invariants such that _deactivatePolicy() no longer works. However, it could also be used for a DoS attack: As _reconfigurePolicies and _migrateKernel iterate over those lists that now contain duplicates, they could run out of gas if a policy is activated enough times.

Recommended Mitigation Steps

Check getPolicyIndex[policy_] != 0 instead of relying on a value of an untrusted contract.

0xLienid (Olympus) commented:

@ind-igo a few other submissions also mention problems with over-reliance on policy.isActive (i.e. #368). Might be worth mitigating with the suggested step here or the check on activePolicies[index] like 368 mentions.

Dupe of #368

I think this is separate from #368 which is about a policy deactivating that isn’t already active.

I am a bit skeptical at the impact statement currently, but it does seem like protocol functionality does end up in a bad state with the typical policy lifecycle here. Will award as Medium unless Sponsor wants to provide some additional reasoning as to a downgrade.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/TRSRY.sol#L64-L102

While the issue is slightly different from #368, the solution is the exact same. The remediation has the new checks to prevent both of these issues.

[M-29] TRSRY susceptible to loan / withdraw confusion

Submitted by Trust, also found by 0xSky, datapunk, and tonisives

Treasury allocates approvals in the withdrawApproval mapping which is set via setApprovalFor(). In both withdrawReserves() and in getLoan(), _checkApproval() is used to verify user has enough approval and subtracts the withdraw / loan amount. Therefore, there is no differentiation in validation between loan approval and withdraw approval. Policies which will use getLoan() (currently none) can simply withdraw the tokens without bookkeeping it as a loan.

Proof of Concept

Policy P has getLoan permission
setApprovalFor(policy, token, amount) was called to grant P permission to loan amount
P calls withdrawReserves(address, token, amount) and directly withdraws the funds without registering as loan

Recommended Mitigation Steps

A separate mapping called loanApproval should be implemented, and setLoanApprovalFor() will set it, getLoan() will reduce loanApproval balance.

ind-igo (Olympus) confirmed, but disagreed with severity and commented:

Confirmed. Good suggestion. Would put as low risk though.

Currently thinking Medium is appropriate for this issue, but will circle back on it.

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

See #293 for a possible vector in which this could lead to loss of funds. Going to leave as Medium.

[M-30] `Heart::beat()` could be called several times in one block if no one called it for a some time

Submitted by rvierdiiev, also found by datapunk, devtooligan, itsmeSTYJ, Jeiwan, Lambda, Trust, and zzzitron

beat() function is allowed to be called by anyone once in frequency() period. The purpose of it is to update the prices and do another operations related to bond market. User who ran it are rewarded. There is no need to run this function more then 1 time in frequency() period. However if beat() was last time called more then frequency() time ago then user can execute beat() function (block.timestamp - lastBeat)/frequency() times in a row in same block and get rewards.

Proof of Concept

Recommended Mitigation Steps

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Heart.sol#L103
Change this line to lastBeat = block.timestamp - (block.timestamp - lastBeat) % frequency();
So no matter how much time the beat() was not called, it is possible to call it only once per frequency().

See comment on #405. This approach actually solves both of our issues though.

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L188-L191
https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L272
https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L346

Going to use this issue as the primary since the solution is elegant and solves the problem.

[M-31] Protocol’s Walls / cushion bonds remain active even if heart is not beating

Submitted by Trust

The Walls of the RBS mechanism offer zero slippage swaps at the high and low of the moving average spread. The capacity to be swapped at these prices is usually very large, so it must make sure to only be enabled when the prices are guaranteed to be synced. However, there is no such check. If beat() is not called for some time, meaning we cannot determine if the current spread is legit, swap() still operates as usual.

Impact

The worst case scenario is that the wall is swapping at a losing price, meaning they can be immediately drained via arbitrage bot.

Proof of concept

1. Price is X at the start
2. Oracle stops updating for some reason / no one calls beat()
3. Price drops to Y , where Y < low wall centered around X
4. Attacker can perform arbitrage by buying Ohm at external markets at Y and selling Ohm at low wall price, netting the difference.

Recommended mitigation steps:

Change modifier onlyWhileActive to add a check for beat out of sync:

if (block.timestamp > lastBeat + SYNC_THRESHOLD * frequency())