Failure of Deposit Function Due to WETH Allowance Handling on Certain Chains

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/L2/xRenzoDeposit.sol#L214

Vulnerability details

Impact

The deposit function may fail on chains with non-standard WETH implementations lacking self-allowance handling.

Proof of Concept

The deposit function in the xRenzoDeposit contract, which is intended to transfer deposit tokens from the user to the contract itself.

    function deposit(
        uint256 _amountIn,
        uint256 _minOut,
        uint256 _deadline
    ) external nonReentrant returns (uint256) {
        if (_amountIn == 0) {
            revert InvalidZeroInput();
        }

        //@audit Transfer deposit tokens from user to this contract
        // weth in arbitrium will not work and user will not deposit
        depositToken.safeTransferFrom(msg.sender, address(this), _amountIn);

        return _deposit(_amountIn, _minOut, _deadline);
    }

However, the transfer done using depositToken.safeTransferFrom. This works fine on most chains (Ethereum, Optimism, Polygon, BSC) which uses the standard WETH9 contract that handles the case when src == msg.sender:

WETH9.sol
        if (src != msg.sender && allowance[src][msg.sender] != uint(- 1)) {
            require(allowance[src][msg.sender] >= wad);
            allowance[src][msg.sender] -= wad;
        }

The problem is that the WETH implementation on Arbitrum , Blast, uses a different contract, and does not have this src == msg.sender handling.

Also, the issue is presented in Wrapped Arbitrum and Wrapped Fantom.

As a result, any attempt to execute this line of code with WETH as the deposit token on Arbitrumc, Blast will fail with an "ERC20: transfer amount exceeds allowance" error, effectively breaking the deposit functionality when users attempt to deposit ETH.

Similar finding.

Tools Used

Manual Review

Recommended Mitigation Steps

 function deposit(
        uint256 _amountIn,
        uint256 _minOut,
        uint256 _deadline
    ) external nonReentrant returns (uint256) {
        if (_amountIn == 0) {
            revert InvalidZeroInput();
        }

--        depositToken.safeTransferFrom(msg.sender, address(this), _amountIn);

++         depositToken.safeTransfer(msg.sender, address(this), _amountIn);

        return _deposit(_amountIn, _minOut, _deadline);
    }

Assessed type

Error

[C4-Staff]

CloudEllie marked the issue as primary issue

[alcueca]

@jatinj615, disputed on what grounds?

[jatinj615]

@alcueca , WETH deposits are possible on L2 with 2 step process. 1. Approve 2. Deposit. Hence, disputed. lmk if I am missing something.

WETH deposit tx on Arbitrum - https://arbiscan.io/tx/0x6c1bdd5a18b1e4cc6e8f72a81f28cd792cbeeb881e4f5f049364bd0d117f6fe4

[alcueca]

Sorry, @jatinj615, I'm reading the report again and it just makes no sense. I don't know where WETH9 comes into this, or why deposit would be a transfer to self.

[c4-judge]

alcueca marked the issue as unsatisfactory: Insufficient quality

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid

[0xAbhay]

@alcueca Hey sir thanks for judging

Issue Explanation: Contract Accounts and Account Abstraction Wallets

Context: The xRenzoDeposit contract uses safeTransferFrom to transfer WETH from the user's account to itself. This method relies on the user having set an allowance for the xRenzoDeposit contract to spend their WETH.

Contract Accounts

Nature: Contract accounts are smart contracts that hold tokens. Allowance Requirement: For `safeTransferFrom` to succeed, the contract account must explicitly set an `allowance` for the `xRenzoDeposit` contract. Failure Scenario: No Allowance Set: If the contract account has not set an `allowance`, the `safeTransferFrom` call will fail with an "ERC20: transfer amount exceeds allowance" error. Example: A DeFi protocol contract tries to deposit WETH into `xRenzoDeposit` without setting an `allowance`. The transaction reverts due to insufficient allowance.

Account Abstraction Wallets (Smart Contract Wallets)

Nature: Account abstraction wallets are smart contract wallets that manage user assets and permissions programmatically. Allowance Management: These wallets need to programmatically set allowances for other contracts to spend their tokens. Failure Scenario: No Allowance Set: If the smart wallet does not set an `allowance` for the `xRenzoDeposit` contract, the `safeTransferFrom` call will fail. Example:A user with Smart Wallet wants to deposit 10 WETH into `xRenzoDeposit`. Steps: The user initiates a deposit. The smart wallet calls `safeTransferFrom` to transfer WETH. The `xRenzoDeposit` contract attempts to execute `safeTransferFrom`. If the smart wallet has not set an allowance for `xRenzoDeposit`, the transaction fails. Result: The transaction reverts with an "ERC20: transfer amount exceeds allowance" error. [ERC4337 account abstraction](https://dune.com/sixdegree/account-abstraction-overview) is widely used, but many smart wallets don't automatically set allowances. This can lead to issues such as deposit failures. Some wallets that don't support automatic allowance include the [Coinbase wallet](https://github.com/coinbase/smart-wallet/blob/main/src/CoinbaseSmartWallet.sol) and the [Ambire Wallet](https://github.com/AmbireTech/wallet/blob/main/contracts/AmbireAccount.sol) and many more.

[JeffCX]

If the smart wallet has not set an allowance for xRenzoDeposit, the transaction fails.

that is user mistake, if there is lack of allowance, transaction is expect to revert, they can always set the allowance to make the deposit succeed.

[0xAbhay]

@alcueca @JeffCX

Clarify the Issue:

• The primary problem is not just the transaction failing due to a lack of allowance, but rather the implications this has on user experience and contract design.

2. Highlight User Experience:

   • From a user's perspective, especially those who are not deeply familiar with the technical workings of smart contracts, the process of manually setting allowances can be confusing and error-prone.
   • In scenarios where the contract does not handle allowance settings programmatically, users may face repeated transaction failures, which can lead to frustration and a lack of trust in the platform.

3. Developer Responsibility:

   • It is essential for developers to anticipate and handle such scenarios programmatically to provide a seamless user experience.
   • Contracts should ideally include mechanisms to check for and set the necessary allowances automatically or guide users through the process efficiently.

4. Examples and Precedents:

   • Provide examples of best practices where contracts handle allowances automatically. Many successful DeFi protocols implement such mechanisms to enhance usability.
   • Reference the widespread use of ERC4337 and its benefits in managing permissions programmatically to avoid manual errors.

Here’s a detailed response:

Thank you for your comments. I would like to provide a detailed explanation to counter your statement and defend my findings.

User Experience and Contract Design

The core issue here is not merely the transaction failure due to a lack of allowance but the implications it has on the overall user experience and the responsibility of smart contract developers to mitigate such failures.

1. User Experience

While it is true that users can set allowances manually, expecting them to do so introduces significant friction. Users, particularly those who are not technically inclined, might find this process confusing and cumbersome. This can lead to repeated transaction failures, which can be frustrating and diminish trust in the platform.

2. Developer Responsibility

From a developer's perspective, it is crucial to anticipate such scenarios and handle them programmatically. Modern smart contracts should include mechanisms to check for existing allowances and set them automatically if needed. This approach not only prevents transaction failures but also ensures a smoother and more intuitive user experience.

3. Best Practices and Examples

Many leading DeFi protocols implement automatic allowance settings to enhance usability. For instance, some protocols use meta-transactions to handle allowance approvals on behalf of the user, thereby reducing the chances of transaction failures.

The widespread adoption of ERC4337 for account abstraction highlights the importance of managing permissions programmatically. Smart wallets that leverage ERC4337, such as the Coinbase Wallet and the Ambire Wallet, are designed to reduce the need for manual user interventions.

Conclusion

In conclusion, while users can set allowances manually, it is imperative for smart contract developers to implement automatic allowance handling mechanisms to ensure a seamless and user-friendly experience. Addressing this issue programmatically can significantly reduce transaction failures and enhance the overall usability of the platform.

I hope this explanation clarifies the importance of addressing allowance settings in smart contracts and why it is a significant consideration for both user experience and contract design.

Thank you for considering my perspective.

[alcueca]

0xAbhay, is it you, or is it chatGPT writing this?

Allowances have been set previous to transfers for the last ten years, protocols and users bear with them in the way they can, and your finding is reporting a UX limitation of Ethereum and ERC20, and not anything specific to Renzo.

[ZanyBonzy]

Hi @0xAbhay, sorry to barge in, I hope I can help clear this a bit.

This issue only occurs if the contract implements the transferFrom like this.

        WETH.safeTransferFrom(address(this), anotherAddress, amountToTransfer);

On some chains, WETH can execute transferFrom directly without approval if msg.sender is also the from. That is, a contract/user can use safeTransferFrom to transfer tokens from themselves without approval. On some other chains, it requires approval, and that is where issue can occur. See this issue for more context.

What you're describing is this, and its different.

      depositToken.safeTransferFrom(msg.sender, address(this), _amountIn);

msg.sender in this case is not the contract, so there's certainly a need for approval. msg.sender will always have to approve the contract to transfer his WETH since the contract is not the from (I hope this makes sense). If a smartcontract or a contract doesn't have the approve function to handle this scenario, it's already out of Renzo's hands as there's nothing they can do.

Also, the mitigation will not work. The transfer method only takes in two arguments, the to and the amount. Adding a third argument will make the function to always fail.

++         depositToken.safeTransfer(msg.sender, address(this), _amountIn);

[0xAbhay]

@alcueca @ZanyBonzy thanks for clearing things out now I understand sorry for the argument 🙏