hats-finance / Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb

Fork of the Inverter Smart Contracts Repository
GNU Lesser General Public License v3.0
0 stars 3 forks source link

BondingCurveBase_v1.sol#_buyOrder() - Computes function selector incorrectly #61

Open hats-bug-reporter[bot] opened 4 months ago

hats-bug-reporter[bot] commented 4 months ago

Github username: -- Twitter username: @EgisSec Submission hash (on-chain): 0xeecfcbb58197b0883b6accc49d4405ad10e34932f2636837974e9fb1857ba6b0 Severity: low

Description: Description\ BondingCurveBase_v1 uses _getFunctionFeesAndTreasuryAddresses to get fee percentages from the FeeManager_v1.

// Get protocol fee percentages and treasury addresses
        (
            address collateralTreasury,
            address issuanceTreasury,
            uint collateralBuyFeePercentage,
            uint issuanceBuyFeePercentage
        ) = _getFunctionFeesAndTreasuryAddresses(
            bytes4(keccak256(bytes("_buyOrder(address, uint, uint)")))
        );

FeeManager_v1 relies on the function selector to correctly guess the return the fees for that specific function and module.

The issue is that _getFunctionFeesAndTreasuryAddresses is called with the incorrect function selector.

The protocol uses: bytes4(keccak256(bytes("_buyOrder(address, uint, uint)"))) = 0xebc8b020,

While the actual selector should be: bytes4(keccak256(bytes("_buyOrder(address,uint256,uint256)"))); = 0xd88e833f.

The second one is the correct one, as _buyOrder function selector is exacltly 0xd88e833f.

If we assume that the owner of setCollateralWorflowFee correctly uses the 0xd88e833f selector, then _buyOrder won't work correctly as getCollateralWorkflowFeeAndTreasury will incorrectly return defaultCollateralFee instead of the real workflow fee that was set.

Attack Scenario\

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

Compute the function selector correctly: return bytes4(keccak256(bytes("_buyOrder(address,uint256,uint256)")));

0xdeth commented 4 months ago

I forgot to add that this will also retrieve the incorrect issuance fee.

This also happens in calculatePurchaseReturn.

PlamenTSV commented 3 months ago

Both issues #61 and #62 are the same vulnerability, no need for separate reports.

0xmahdirostami commented 3 months ago

Due to tests, it seems that owner uses bytes4(keccak256(bytes("_buyOrder(address, uint, uint)"))) = 0xebc8b020, this one.

Check testInternalGetBuyFeesAndTreasuryAddresses_works.

0xdeth commented 3 months ago

@0xmahdirostami

The owner can be anyone, they will compute the function signature correctly (without the spaces). It's fair to assume that.

It's not a big issue, but I think it's at least a low.

0xmahdirostami commented 3 months ago

The owner can be anyone, they will compute the function signature correctly (without the spaces

yes you are right. thanks for commenting back.

we could always miss something