Execution would fail when tokens like weth is being transferred out in V3Automation on Arbitrum

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Automation.sol#L91-L168

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Automation.sol#L91-L168

    function _execute(ExecuteParams calldata params, address positionOwner) internal {
..snip
        params.nfpm.transferFrom(address(this), positionOwner, params.tokenId);
    }

Evidently at the final step of the execution this is being done.

NB: A similar logic could be applicable here.

Now note that from the readMe the below has been stated https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/README.md#L101

| Question                                | Answer                                      |
| --------------------------------------- | ------------------------------------------- |
| ERC20 used by the protocol              |       Any             |
| Chains the protocol will be deployed on | Ethereum,Arbitrum,Base,BSC,Optimism,Polygon |

Now whereas this would work normally on other chains if WETH or tokens like it is integrated and is being transferrred out, it would fail on the Arbitrum chain, which protocol plans to deploy on, this is because the transfer is transferFrom and then passing addressThis as the from, as hinted earlier on this would work fine on most chains (Ethereum, Optimism, Polygon, BSC) which uses the standard WETH9 contract that handles the case when src == msg.sender, i.e:

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 uses a different contract, and does not have this src == msg.sender handling, which makes the transfer attempt to always fail.

Also, the issue is present both on Blast and Wrapped Fantom. ...just incase protocol decides on deploying there in the future.

Impact

Because the contract never approves itself to spend WETH, the token transfer line will always revert on Arbitrum when dealing with WETH

Categorizing this as Medium given that it impacts the availability of V3Automation#execute() on the supported Arbitrum chain with WETH.

Tool used

• Manual review
• Past findings: 1 & 2.

Recommended Mitigation Steps

    function _execute(ExecuteParams calldata params, address positionOwner) internal {
..snip
-        params.nfpm.transferFrom(address(this), positionOwner, params.tokenId);
+        params.nfpm.transfer(positionOwner, params.tokenId);
    }

Assessed type

Token-Transfer

[3docSec]

Provisionally marking as satisfactory

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report

[3docSec]

This finding seems to come from a misunderstanding: params.nfpm is an NFT contract, so it can't point to an ERC20 contract like WETH9

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid