LTDingZhen - Keepers can be forced to waste gas with a modified `onERC721Received()`

[sherlock-admin2]

LTDingZhen

high

Keepers can be forced to waste gas with a modified onERC721Received()

Summary

The onERC721Received() function of the contract address is called when the keeper tries to mint a new position to the user by executeOpen. This allows a malicious user to frontrun keeper's transactions to self-destruct the contract and deploy new malicious bytecode to grief the keeper.

Vulnerability Detail

When calculating the value of Keeperfee, _gasUnitsL2 and _gasUnitsL1 are used as an estimate of the gas required to execute the order:

uint256 costOfExecutionGrossEth = ((((_gasUnitsL1 + overhead) * l1BaseFee * scalar) / 10 ** decimals) + (_gasUnitsL2 * gasPriceL2));

So, Keeper does not get paid more for consuming more gas.

Consider such attack path:

1. Attacker set up an normal contract wallet with onERC721Received() and self-destruct function.
2. Attacker announceOpen a position, start listening for keeper transactions.
3. A Keeper notice the order and calculated it to be a profitable deal, send the tx into mempool.
4. Attacker frontrun the order, self-destruct his contract on the address and re-deploy one with malicious onERC721Received(), which execute complex logic until the gaslimit is reached.
5. Attacker repeat the above steps until the Keeper's ETH balance is drained.

Impact

Since keeper is automatically executed by the script, attacker can drain ALL Keeper's balance through the above attack path at 0 cost.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L111 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/misc/KeeperFee.sol#L104-L105

Tool used

Manual Review

Recommendation

Consider using something like GasUtils on GMX to validate execution Gas and make sure it is not too big.

[nevillehuang]

Request PoC

[sherlock-admin2]

PoC requested from @RealLTDingZhen

Requests remaining: 9

[sherlock-admin]

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

0xLogos commented:

no front-running on base

takarez commented:

invalid: attacker looses gas too

[RealLTDingZhen]

Hi @nevillehuang, Throughout the workflow of opening a leveraged position, Keeperfee is only roughly estimated when the user calls announceLeverageOpen, and is not examined in the actual execution of executeOpen. The problem is that OZ's safemint calls a contract's onERC721Received() to check whether the execution was successful or not. (without gas check)

function checkOnERC721Received(
    address operator,
    address from,
    address to,
    uint256 tokenId,
    bytes memory data
) internal {
    if (to.code.length > 0) {
        try IERC721Receiver(to).onERC721Received(operator, from, tokenId, data) returns (bytes4 retval) {
            if (retval != IERC721Receiver.onERC721Received.selector) {
                // Token rejected
                revert IERC721Errors.ERC721InvalidReceiver(to);
            }
        } catch (bytes memory reason) {
            if (reason.length == 0) {
                // non-IERC721Receiver implementer
                revert IERC721Errors.ERC721InvalidReceiver(to);
            } else {
                /// @solidity memory-safe-assembly
                assembly {
                    revert(add(32, reason), mload(reason))
                }
            }
        }
    }
}

Even though keepers can choose to execute and order or leave it depending on if the order is profitable for them (execution cost is less than keeper fee) or not, attacker can grief keepers by changing the state of a contract before keepers‘ transaction is executed. (After their simulation offchain)

It is worth pointing out that this attack doesn't rely on frontrunning. Since keepers are bots, an attacker can always figure out the regularity of execution times and make the onERC721Received() to change in the block where the order is executed by keeper.

[nevillehuang]

@RealLTDingZhen, your original submission only pointed out a front-running scenario which is not possible based on comments in #49 given users are expected to interact with protocol on base chain directly, so I am inclined to invalidate this issue.

[RealLTDingZhen]

Hi @nevillehuang I don't rely on mempool for my "frontrun" here. What I'm trying to say is to execute the attack logic between Keepers' off-chain simulation and on-chain execution. Since Keepers are bots, we can always expect them to perform transactions according to a certain pattern, so it is feasible to make a rough guess as to when keeper submitted the transaction.

In addition, attackers can also tries to offer a high Keeperfee to make keeper execute their offer instantly.

[rashtrakoff]

@RealLTDingZhen I don't think this is something we can fix. The keepers (at least the ones we run) do estimate the gas and only execute orders when the provided keeper fee is profitable enough for them. Also, the keeper after estimation sets a gas limit for the transaction it's sending so in case the execution costs more than the keeper fee, the transaction will simply revert. Let us know if you can see a solution to this.