ūüďĄ

Report #13262

Report Date
November 7, 2022

Unbounded gas consumption in `beanstalkMint()` leads to griefing

‚Ä£
Report Info

Report ID

#13262

Target

Report type

Smart Contract

Impacts

  • Theft of gas
  • Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)
  • Unbounded gas consumption

Has PoC?

Yes

Bug Description

  • In¬†[Fertilizer.sol](https://etherscan.io/address/0x39cdaf9dc6057fd7ae81aaed64d7a062aaf452fd)¬†there is a function¬†beanstalkMint(address, uint, uint128, uint128)¬†which invokes internal¬†_safeMint(address, uint, uint, bytes)¬†in order to mint ERC1155 tokens to specified user. After minting tokens¬†_transfer(address(0), to, id, amount)¬†we can see extra checks¬†__doSafeTransferAcceptanceCheck(address, address, address, uint, uint, bytes)¬†whether the recipient is able to handle with tokens in case if it's not EOA. However, malicious Bob(nothing personal) can simply put huge amount redundant code in his¬†onERC1155Received()¬†which wastes the gas till hard limit(he can precompute that) in order to grief the owner, who is trying to mint some tokens to him.

Impact

  • Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)
  • Theft of gas
  • Unbounded gas consumption

Risk Breakdown

  • Difficulty to Exploit: Medium
  • CVSS2 Score: 5.0

Recommendation

  • Short term: just use custom¬†_mint()¬†without an acceptance checks. I'm pretty sure that the users who're using SC to receive tokens are fully aware of such token-handling problems. And also, it's pretty much optimized. Applying this recommendation protocol will deliver the promised value, but everything else relies on the user side.
  • Long term: maybe designing a mechanism that allocates certain amount of gas per call?

Proof of concept

  • I wanted to set up some various simulations using tenderly, but didn't see too much value from it, since everything described above relies on a unbounded gas consumption.
  • Spent some time on understanding the reason behind not implementing proper¬†initilize()¬†function which will invoke¬†__Internallize_init()¬†from the base contract. I was able to discrover proxyAdmin(via internal transactions on a proxy side) in order to look for previous implementations and realized that you had fertilizerPreMint which actually had¬†initialize()¬†in it.
  • The question is: "What was the reason behind removing¬†initialize()¬†from implementation?"

And also, take a look at _delegate() which doesn't have any gas limits for delegate calls, therefore the gas griefing is possible.

 function _delegate(address implementation) internal virtual {
        // solhint-disable-next-line no-inline-assembly
        assembly {
            // Copy msg.data. We take full control of memory in this inline assembly
            // block because it will not return to Solidity code. We overwrite the
            // Solidity scratch pad at memory position 0.
            calldatacopy(0, 0, calldatasize())

            // Call the implementation.
            // out and outsize are 0 because we don't know the size yet.
            let result := delegatecall(gas(), implementation, 0, calldatasize(), 0, 0)

            // Copy the returned data.
            returndatacopy(0, 0, returndatasize())

            switch result
            // delegatecall returns 0 on error.
            case 0 { revert(0, returndatasize()) }
            default { return(0, returndatasize()) }
        }
    }

BIC Response

This is not a security bug report because the issue requires a user to mint Fertilizer to a contract that is intended to waste gas.

Due to this reason, the BIC is closing the submission and no reward will be issued.

Reporter Response

I think, Beanstalk team misunderstood the report a little, since the user is not required to mint any Fertilizer to contract, but instead the owner invokes beanstalkMint(address, uint, uint128, uint128) in order to mint ERC1155 tokens to arbitrary receiver.

Let's try to look again into the following code:

function __doSafeTransferAcceptanceCheck(
        address operator,
        address from,
        address to,
        uint256 id,
        uint256 amount,
        bytes memory data
    ) private {
        if (to.isContract()) {
            try IERC1155ReceiverUpgradeable(to).onERC1155Received(operator, from, id, amount, data) returns (bytes4 response) {
                if (response != IERC1155ReceiverUpgradeable.onERC1155Received.selector) {
                    revert("ERC1155: ERC1155Receiver rejected tokens");
                }
            } catch Error(string memory reason) {
                revert(reason);
            } catch {
                revert("ERC1155: transfer to non ERC1155Receiver implementer");
            }
        }
    }

Here we're invoking to.onERC1155Received(), where to - some user's SC. Basically, there are no any gas limits allocated to make this call happen and the user simply could drain the gas till the hard limit per block, therefore we can say for sure, that the impact listed on a Beanstalk main page (Unbounded gas consumption) makes sense in the context above.

Beanstalk team did a great job by simply providing the context where beanstalkMint() could be invoked. Unfortunately, FertilizerFacet.sol wasn't listed on Immunefi and I didn't see the clear context.

This report is now closed.

BIC Response

  1. beanstalkMint() is only callable by the Fertilizer address, and is only used in line 58 of FertilizerFacet.sol: C.fertilizer().beanstalkMint(msg.sender, uint256(id), amount, s.bpf);. Therefore the receiver and the caller are the same person. Thus, while the receiver may very well do what is described in the bug, they would only be harming themselves.
  2. This exploit could be harmful to the user if they increased the gas limit in their transaction, but this isn't something that Beanstalk can control.
  3. While appreciate every bug report is appreciated, a POC is required, ideally one that forks mainnet and shows the economic damage. In the reported recommendation, it is explicitly stated that "users who are using smart contracts are fully aware of token-handling problems", meaning that in practice, the issue that was elevated is common practice in the space. In general, the use of mint() vs safeMint() is somewhat of a controversial argument (why do the additional computation to ensure the wallet is able to receive ERC1155's where 99% of the time they are able to), as Fertilizer is a temporary asset for Beanstalk, as well as it is a one-time transaction for many people, think that the reward outweighs the cost.