Token Withdrawal Module for token withdrawals on behalf of IPAccount

[jdubpark]

Beta-release patch that allows IPAccounts to call some external ERC-20/721/1155 contracts to transfer out the tokens it received, e.g. from the Royalty Module.

Because #127 requires either the signer or to be a registered module, we must use a module to transfer tokens from IPAccount. Thus, we propose the Token Management Module.

Currently, there is a bug:
An IPAccount can delegate to a frontend contract (not a registered module) to transfer tokens on behalf of the IPAccount via the Token Management Module. This can be a Web2-native user who doesn't know how to transfer tokens, thus grants the Web2 app to move around tokens on blockchain.

The bug is that this frontend contract can transfer any tokens that are approved by the IPAccount for the Token Management Module. Let's go with an example:

1. IPAccount gets ERC-20 USDC and WETH from royalty payment.
2. IPAccount wants to move both tokens, so it approves Token Management Module to transfer both USDC and WETH on behalf of it.
3. IPAccount wants a new DEX to transfer only USDC from its account. IPAccount approves the DEX to call the Token Management Module to transfer token.
4. The new DEX now has power to move both USDC and WETH.

In other words, there's no mechanism for the Token Management Module to granularly control which token a caller can move. If a caller is an IPAccount owner (crypto native), then it's fine. But if it's a frontend contract that is acting on behalf of an IPAccout (because the owner is lazy or not crypto-native), then it can get quite dangerous to due this catch-all permission for token transfers.

See test "test_TokenManagementModule_transferERC20_revert_malicious_anotherERC20Transfer", which doesn't actually revert as expected (so malicious frontend transfers both tokens).

[openzeppelin-code]

Token Management Module for token transfers on behalf of IPAccount

Generated at commit: 231864eb49def13ca34cf2f3b28e8e86df30ddfa

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 1 0 5 32 39
Dependencies	Critical High Medium Low Note Total	0 2 0 1 85 88

---

For more details view the full report in OpenZeppelin Code Inspector

[LeoHChen]

add proper comments please, also please call out the security risk in the code and PR messages.

[kingster-will]

Currently, there is a bug:
An IPAccount can delegate to a frontend contract (not a registered module) to transfer tokens on behalf of the IPAccount via the Token Management Module. This can be a Web2-native user who doesn't know how to transfer tokens, thus grants the Web2 app to move around tokens on blockchain.

If we consider this is a potential risk, then why not just refactor the Module to restrict the module can only transfer the tokens from IPAccount to the IPAccount owner address?

in implementation, just removed the parameter to from the transferERC20() interface. Hence, the caller cannot specify the recipient of the token and hardcode the recipient is IPAccount owner.

   function transferERC20(
        address payable ipAccount,
        address tokenContract,
        uint256 amount
    ) external verifyPermission(ipAccount) {
        IIPAccount(ipAccount).execute(
            tokenContract,
            0,
            abi.encodeWithSignature("transfer(address,uint256)",  IIPAccount(ipAccount).owner(), amount)
        );
    }

[kingster-will]

This module should not be in the core repo, it should be created in periphery repo: https://github.com/storyprotocol/protocol-periphery/

[jdubpark]

re: periphery @kingster-will
At this point, we can't put a module in periphery without migrating a lot of other code that also belongs in periphery. It was either way before or later when we started working on post-beta. For beta, let's just keep it in one place to unify our codebase.

re: to set to IPAccount owner
Sure, this works, since a "drainer" can transfer tokens only to IPAccount owner. But this won't work post-beta, because this makes IPAccount less programmable. Once tokens are transferred to IPAccount owner, it introduces layers of complexity to move tokens from IPAccount to other contracts, for programmability.

[jdubpark]

re-re: periphery

I definitely agree this should go on periphery, but it's mostly how much extra work we need to do:

1. Rewrite the test to work in periphery as the same in core.
2. Add a new deployment script in Periphery that deploys this module with the latest updated contract addresses.
3. Add new docs to guide devs of what "periphery" is what contracts are in there.

[Ramarti]

Currently, there is a bug:
An IPAccount can delegate to a frontend contract (not a registered module) to transfer tokens on behalf of the IPAccount via the Token Management Module. This can be a Web2-native user who doesn't know how to transfer tokens, thus grants the Web2 app to move around tokens on blockchain.

If we consider this is a potential risk, then why not just refactor the Module to restrict the module can only transfer the tokens from IPAccount to the IPAccount owner address?
in implementation, just removed the parameter to from the transferERC20() interface. Hence, the caller cannot specify the recipient of the token and hardcode the recipient is IPAccount owner.

   function transferERC20(
        address payable ipAccount,
        address tokenContract,
        uint256 amount
    ) external verifyPermission(ipAccount) {
        IIPAccount(ipAccount).execute(
            tokenContract,
            0,
            abi.encodeWithSignature("transfer(address,uint256)",  IIPAccount(ipAccount).owner(), amount)
        );
    }

Not sure this solves the issue at hand. Given the drainer implements the same interface but might not even use the inputs that are passed in. Or am I missing something?

Only mitigation for that is to use the whitelist Royalty has for ERC20s.
This is a patch, but we need to converge post beta if this is a barebones wallet, a module, a modular wallet, and properly review assumptions.

[jdubpark]

This Transfer Module also enforces "whitelist" by making each IPAccount approve for a token.

Note, this becomes problematic for Royalty NFTs because each RNFT is a different ERC-1155 contract (0xSplits requirement). Every IPAccount has to approve a different 1155 address for itself, really bad UX and DX.

Consider a case: Frontend App manages 1 million IPAccounts (users) for royalty-related licensing. For each IPAccount, it must request the user to sign (or call directly) to approve Transfer Module to transfer its RNFT, and then another tx to delegate Transfer Module to Frontend App.

[kingster-will]

Currently, there is a bug:
An IPAccount can delegate to a frontend contract (not a registered module) to transfer tokens on behalf of the IPAccount via the Token Management Module. This can be a Web2-native user who doesn't know how to transfer tokens, thus grants the Web2 app to move around tokens on blockchain.

If we consider this is a potential risk, then why not just refactor the Module to restrict the module can only transfer the tokens from IPAccount to the IPAccount owner address?

in implementation, just removed the parameter to from the transferERC20() interface. Hence, the caller cannot specify the recipient of the token and hardcode the recipient is IPAccount owner.

   function transferERC20(
        address payable ipAccount,
        address tokenContract,
        uint256 amount
    ) external verifyPermission(ipAccount) {
        IIPAccount(ipAccount).execute(
            tokenContract,
            0,
            abi.encodeWithSignature("transfer(address,uint256)",  IIPAccount(ipAccount).owner(), amount)
        );
    }

With this solution, we can rename the module to reflect better what it does.
Rename the module to TokenWithdrawModule and function rename to withdrawERC20(), withdrawERC721() and withdrawERC1155().

[LeoHChen]

Currently, there is a bug:
An IPAccount can delegate to a frontend contract (not a registered module) to transfer tokens on behalf of the IPAccount via the Token Management Module. This can be a Web2-native user who doesn't know how to transfer tokens, thus grants the Web2 app to move around tokens on blockchain.

If we consider this is a potential risk, then why not just refactor the Module to restrict the module can only transfer the tokens from IPAccount to the IPAccount owner address?

in implementation, just removed the parameter to from the transferERC20() interface. Hence, the caller cannot specify the recipient of the token and hardcode the recipient is IPAccount owner.

   function transferERC20(
        address payable ipAccount,
        address tokenContract,
        uint256 amount
    ) external verifyPermission(ipAccount) {
        IIPAccount(ipAccount).execute(
            tokenContract,
            0,
            abi.encodeWithSignature("transfer(address,uint256)",  IIPAccount(ipAccount).owner(), amount)
        );
    }

like this suggestion to reduce risk with simple solution

[jdubpark]

Okay, pushing a commit to reflect changes in the discussions made above. We seem to be settled on our final decision @Spablob @LeoHChen @kingster-will @Ramarti

We are still facing UX/DX issue of "every IPAccount needs to approve a unique 1155 token address for their RNFTs, if commercial." This is a business problem, not a technical one, but I predict this will make the life of external devs much worse when it comes to royalty (which is already quite a lot).

[jdubpark]

Additionally, IPAssets (NFT + IPAccount) are much less programmable if there's only a withdrawal function for tokens. This should not be our solution for the post-beta decision, in my opinion.

[leeren]

Agree with the short-term decision. It is the same as a SeaPort conduit, which also abstracts transfers based on the type of transfer being invoked. We could take more inspiration from them in the future.

Longer term, however, I agree we should solve the UX problem of having to independently approve every new royalty NFT that is used by an IP account.

[kingster-will]

We need to keep the MockTokenManagementModule, the TokenWithdrawalModule` will be moved to the periphery repo. And we should cover such tests in core repo.

[kingster-will]

LGTM.
Do we need to add the Modules into deployment scripts so that we can deploy the module with other components?