Lack of ERC1155 Token Withdrawal Functionality in VirtualAccount Contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L17

Vulnerability details

Summary

The issue at hand pertains to the absence of functionality for users to withdraw ERC1155 tokens from the VirtualAccount contract. While the contract correctly implements the onERC1155Received and onERC1155BatchReceived functions, enabling it to receive ERC1155 tokens, it lacks a corresponding withdrawal mechanism for these tokens. In contrast, other token types, such as native Ether, ERC20, and ERC721 tokens, have dedicated withdrawal functions in the contract (withdrawNative, withdrawERC20, and withdrawERC721). However, ERC1155 tokens have not been considered in the withdrawal process.

Impact

This limitation means that users who have deposited ERC1155 tokens into their virtual accounts have no means to retrieve them. The absence of this functionality can be highly inconvenient for users who may need to manage and utilize these ERC1155 tokens on other platforms or within other contracts. Consequently, users may perceive this limitation as a significant drawback, negatively affecting the overall utility and user experience of the contract.

Tools Used

Manual

Recommended Mitigation Steps

To address this issue and empower users to withdraw ERC1155 tokens from their virtual accounts, it is recommended to implement a dedicated withdrawal function specifically designed for ERC1155 tokens.
Below is a suggested solution for such a function:

function withdrawERC1155(address _token, uint256[] memory _ids, uint256[] memory _amounts) external requiresApprovedCaller {
    require(_ids.length == _amounts.length, "Array lengths do not match");
    IERC1155(_token).safeBatchTransferFrom(address(this), msg.sender, _ids, _amounts, "");
}

By implementing this proposed solution, users will have the capability to withdraw ERC1155 tokens, enhancing the versatility and user-friendliness of the contract.

Assessed type

DoS

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-sponsor]

0xLightt marked the issue as disagree with severity

[c4-sponsor]

0xLightt (sponsor) disputed

[0xLightt]

Funds are never at risk without it. Users can always use call() to interact with funds. Adding a withdrawERC1155 can improve UX, but there is no issue or any kind of risk if it does not exist.
The reason behind the dispute is that this and duplicate findings state that virtual accounts have no means to retrieve them, which is incorrect.

[alcueca]

Given that users can retrieve their funds (and that MaiaDAO can implement a frontend to make it easy), this is QA

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a