LM_KPIRewarder_v1.sol#postAssertion() - If `stakingToken != defaultCurrency` and `asserter == address(this)`, when an assertion is settled successfully all funds will be refunded back to `address(this)` permanently freezing them

[hats-bug-reporter[bot]]

Github username: -- Twitter username: @EgisSec Submission hash (on-chain): 0x394cd36d799f0799e466aadbfdef47056f3dd861bf1769bbc74599f70ff83033 Severity: high

Description: Description\ postAssertion does allow for asserter == address(this) only when stakingToken != defaultCurrency.

//  If the asserter is the Module itself, we need to ensure the token paid for bond is different than the one used for staking, since it could mess with the balances
        if (
            asserter == address(this)
                && address(defaultCurrency) == stakingToken
        ) {
            revert
                Module__LM_PC_KPIRewarder_v1__ModuleCannotUseStakingTokenAsBond();
        }

This means that if the tokens are different, the asserter can be address(this).

The asserter is used by OOv3, as the address that bond is supposed to be transferred to, when an assertion is settled successfully, this also happens in a disputed assertion and the disputer lost.

 function settleAssertion(bytes32 assertionId) public nonReentrant {
        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 {
            // Dispute, settle with the disputer. Reverts if price not resolved.
            int256 resolvedPrice = _oracleGetPrice(assertionId, assertion.identifier, assertion.assertionTime);

            // If set to discard settlement resolution then false. Else, use oracle value to find resolution.
            if (assertion.escalationManagerSettings.discardOracle) assertion.settlementResolution = false;
            else assertion.settlementResolution = resolvedPrice == numericalTrue;

            address bondRecipient = resolvedPrice == numericalTrue ? assertion.asserter : assertion.disputer;

            // Calculate oracle fee and the remaining amount of bonds to send to the correct party (asserter or disputer).
            uint256 oracleFee = (burnedBondPercentage * assertion.bond) / 1e18;
            uint256 bondRecipientAmount = assertion.bond * 2 - oracleFee;

            // Pay out the oracle fee and remaining bonds to the correct party. Note: the oracle fee is sent to the
            // Store contract, even if the escalation manager is used to arbitrate disputes.
            assertion.currency.safeTransfer(address(_getStore()), oracleFee);
            assertion.currency.safeTransfer(bondRecipient, bondRecipientAmount);

            if (!assertion.escalationManagerSettings.discardOracle)
                _callbackOnAssertionResolve(assertionId, assertion.settlementResolution);

            emit AssertionSettled(assertionId, bondRecipient, true, assertion.settlementResolution, msg.sender);
        }
    }

You can see that in the if, bond is transferred to asserter and in the else, bondRecipientAmount is transferred to bondRecipient, who can be the asserter.

The issue here is that if these tokens are refunded to address(this), the tokens are permanently stuck in the contract.

This happens because OptimisticOracleIntegrator.sol#assertDataFor, always forces _msgSender to transfer defaultBond when the function is called and the function never forces address(this) to use it's own funds if it has them, so refunded bonds will be permanently stuck in the contract.

One way to think is that if defaultBond == 0, then the contract will use it's own funds, this is impossible as _setDefaultCurrencyAndBond will revert, as 0 < oo.getMinimumBond(address(_newCurrency)).

 function _setDefaultCurrencyAndBond(address _newCurrency, uint _newBond)
        internal
    {
        if (address(_newCurrency) == address(0)) {
            revert Module__OptimisticOracleIntegrator__InvalidDefaultCurrency();
        }
        if (_newBond < oo.getMinimumBond(address(_newCurrency))) {
            revert Module__OptimisticOracleIntegrator__CurrencyBondTooLow();
        }

        defaultCurrency = IERC20(_newCurrency);
        defaultBond = _newBond;
    }

Even if it was possible, it would be impossible to create new assertions, as when oo.assertTruth is called, defaultBond is always used so that won't work as well.

Attack Scenario\

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

The fixes could be several.

1. Allow address(this) to pay for assertions, thus don't always force _msgSender to pay the defaultBond.
2. Have an emergency admin function, which transfers tokens out of the contract.

[0xNuggan]

This one is a duplicate of number 64 .Funds are never truly stuck in a module, but it's true that the bond would not be taken from the expected address