bin2chen - keep() may not be able to execute

[sherlock-admin]

bin2chen

medium

keep() may not be able to execute

Summary

Kept.keep() calls _raiseKeeperFee() to get the token Most current implementations of _raiseKeeperFee() remove the trailing number 18 -> 6 -> 18. However, when Kept.keep() is transferred to msg.sender, the trailing number is still included, which may leads to transfer failure

Vulnerability Detail

Kept.keep() calls _raiseKeeperFee() to get the token

    modifier keep(UFixed18 multiplier, uint256 buffer, bytes memory data) {
        uint256 startGas = gasleft();

        _;

        uint256 gasUsed = startGas - gasleft();
        UFixed18 keeperFee = UFixed18Lib.from(gasUsed)
            .mul(multiplier)
            .add(UFixed18Lib.from(buffer))
            .mul(_etherPrice())
            .mul(UFixed18.wrap(block.basefee));
@>      _raiseKeeperFee(keeperFee, data);

           keeperToken().push(msg.sender, keeperFee);

Currently both PythOracle and MultiInvoker implement the _raiseKeeperFee() method.

Let's take MultiInvoker_raiseKeeperFee() for example

contract MultiInvoker is IMultiInvoker, Kept {
...
    function _raiseKeeperFee(UFixed18 keeperFee, bytes memory data) internal override {
        (address account, address market, UFixed6 fee) = abi.decode(data, (address, address, UFixed6));
        if (keeperFee.gt(UFixed18Lib.from(fee))) revert MultiInvokerMaxFeeExceededError();

        IMarket(market).update(
            account,
            UFixed6Lib.MAX,
            UFixed6Lib.MAX,
            UFixed6Lib.MAX,
@>          Fixed6Lib.from(Fixed18Lib.from(-1, keeperFee), true),
            false
        );

    }

contract Market is IMarket, Instance, ReentrancyGuard {
...
    function _update(
        Context memory context,
        address account,
        UFixed6 newMaker,
        UFixed6 newLong,
        UFixed6 newShort,
        Fixed6 collateral,
        bool protect
    ) private {
..
        if (collateral.sign() == 1) token.pull(msg.sender, UFixed18Lib.from(collateral.abs()));
@>      if (collateral.sign() == -1) token.push(msg.sender, UFixed18Lib.from(collateral.abs()));

        // events
        emit Updated(account, context.currentTimestamp, newMaker, newLong, newShort, collateral, protect);
    }

We can see that _raiseKeeperFee() does an interception of the decimal places, removing the ones after 6. 18 -> 6 -> 18.

But keep() still takes the full number of decimal places.

    modifier keep(UFixed18 multiplier, uint256 buffer, bytes memory data) {
        uint256 startGas = gasleft();

        _;

        uint256 gasUsed = startGas - gasleft();
         UFixed18 keeperFee = UFixed18Lib.from(gasUsed)
            .mul(multiplier)
            .add(UFixed18Lib.from(buffer))
            .mul(_etherPrice())
            .mul(UFixed18.wrap(block.basefee));
         _raiseKeeperFee(keeperFee, data);

@>       keeperToken().push(msg.sender, keeperFee);

This causes _raiseKeeperFee() to get the number of tokens that may be less than keeperFee causing keeperToken().push(msg.sender, keeperFee); to fail

Impact

keep() may not perform properly

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/root/contracts/attribute/Kept.sol#L54C1-L54C1

Tool used

Manual Review

Recommendation

Keep() removes decimals after the 6th digit, as usual.

    modifier keep(UFixed18 multiplier, uint256 buffer, bytes memory data) {
        uint256 startGas = gasleft();

        _;

        uint256 gasUsed = startGas - gasleft();
        UFixed18 keeperFee = UFixed18Lib.from(gasUsed)
            .mul(multiplier)
            .add(UFixed18Lib.from(buffer))
            .mul(_etherPrice())
            .mul(UFixed18.wrap(block.basefee));
+       keeperFee = UFixed18Lib.from(UFixed6Lib.from(keeperFee,true));
        _raiseKeeperFee(keeperFee, data);

        keeperToken().push(msg.sender, keeperFee);

        emit KeeperCall(msg.sender, gasUsed, multiplier, buffer, keeperFee);
    }

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

141345 commented:

m

darkart commented:

Keepers take their rewards in Wei so 18 Decimal is correct

[arjun-io]

When going from 18 decimals -> 6 decimals the values are rounded up which will not lead to amounts where the credited amount is greater than the debited amount.

[sherlock-admin2]

Escalate

Please take a look at this description of the problem again, which is not described as leading to "amounts where the credited amount is greater than the debited amount."

What this describes is that keep() -> keeperToken().push(msg.sender, keeperFee); will fail because there is not enough balance in the contract.

Let's take MultiInvoker.sol as an example, when the user executes _executeOrder() there will be the following keeperToken balance changes

1. Initially MultiInvoker.sol's keeperToken balance is 0.

2. Assuming that the keeperFee = 1_000000_123456789012 calculated in keep() is 18 digits. This formula is likely to produce tails larger than 6 digits.

       UFixed18 keeperFee = UFixed18Lib.from(gasUsed)
           .mul(multiplier)
           .add(UFixed18Lib.from(buffer))
           .mul(_etherPrice())
           .mul(UFixed18.wrap(block.basefee));

3. Execute _raiseKeeperFee(keeperFee, data); , this method to go to the market and get the balance Since this method is 18 -> 6 -> 18, it only gets back keeperFee = 1_000000_0000000000, the mantissa is removed. At this point, the balance of MultiInvoker.sol is 1_00000000_0000000000.

4. keep() executes keeperToken().push() method to transfer the token to msg.sender, the quantity passed in this method is still 1_000000_123456789012 with full mantissa.

That is, keeperToken().push(msg.sender, 1_000000_123456789012);

But at this point, the contract balance is actually only 1_000000_0000000000. The balance is not enough, so the transfer will fail.

I don't know if this is clear.

You've deleted an escalation for this issue.