hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4

Other
1 stars 3 forks source link

`increaseBounty` gas check is too strict #44

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

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

Github username: @ShaheenRehman Twitter username: 0x_Shaheen Submission hash (on-chain): 0xf722d4bc8c7aaa32eb8635214ecd23c2e5608ae267daebf33804d46a498d1a33 Severity: low

Description: Description\ The GeneralisedIncentives.increaseBounty have a check which make sure users have sent exact gas as the computed sum:

if (msg.value != sum) revert IncorrectValueProvided(sum, uint128(msg.value));

And this is how sum is computed:

        uint128 maxDeliveryFee = incentive.maxGasDelivery * deliveryGasPriceIncrease;
        uint128 maxAckFee = incentive.maxGasAck * ackGasPriceIncrease;
        uint128 sum = maxDeliveryFee + maxAckFee;

As the sum is computed with the storage variables & input params multiplications and additions, it will be hard for users to input the same exact gas as the sum value.

User's Trxs will revert with the IncorrectValueProvided() and they will lose gas (on Ethereum, it's quite costy for even a fail trx)

Impact\ users will have hard time increasing the bounty due to a too strict check

Code Link https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/IncentivizedMessageEscrow.sol#L130

Proof of Concept The Protocol written test of increaseBounty also showcase this, that before sending a Trx, user needs to do all the computation, for the Trx to go thru:

    function test_increase_escrow() public {
        uint64 increaseAck = 123123;
        uint64 increaseDelivery = 321321;

        bytes32 messageIdentifier = submitMessage(_MESSAGE);

        uint128 deliveryGas = _INCENTIVE.maxGasDelivery * increaseDelivery;
        uint128 ackGas = _INCENTIVE.maxGasAck * increaseAck;
        uint128 difference = deliveryGas + ackGas;

        escrow.increaseBounty{value: difference}(
            messageIdentifier,
            increaseDelivery,
            increaseAck
        );
    }

Mitigation There are two mitigations:

  1. Allow users to overpay and then refund, this is easy, the users will get repaid just like in the submitMessage func:
    if (msg.value < sum) revert IncorrectValueProvided(sum, uint128(msg.value));
  2. Add a view function to calculate the increaseBounty Gas:

    function getIncreaseBountySum(
        bytes32 messageIdentifier,
        uint96 deliveryGasPriceIncrease,
        uint96 ackGasPriceIncrease
    ) external payable returns(uint128) {
        if (_bounty[messageIdentifier].refundGasTo == address(0)) revert MessageDoesNotExist();
    
        IncentiveDescription storage incentive = _bounty[messageIdentifier];
    
        // Compute incentive metrics.
        uint128 maxDeliveryFee = incentive.maxGasDelivery * deliveryGasPriceIncrease;
        uint128 maxAckFee = incentive.maxGasAck * ackGasPriceIncrease;
        return uint128 sum = maxDeliveryFee + maxAckFee;
     }
reednaa commented 5 months ago

There is no flaky behavior here. If you did everything correctly, the only way the call would fail on-chain is if someone sniped your increaseBounty.

It is simple to read storage: https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/IncentivizedMessageEscrow.sol#L87-L89

It is not like it is changing often.

By failing strict you can save gas by simulating the execution off-chain and only submitting when it goes through. This lets you save a transfer.