Maia DAO - Ulysses - Vagner's results

Lines of code

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

Vulnerability details

Impact

payableCall which can be found in VirtualAccount.sol does not use the requiresApprovedCaller modifier, like every other function in the contract does, which will let anyone do arbitrary calls from any VirtualAccount leading to loss of funds/assets to the users.

Proof of Concept

As you can see in the contract every function uses requiresApprovedCaller which ensures that the functions can only be called by the owner of the VirtualAccount or the approved router, to make changes like transferring ERC20, Native tokens or ERC721 from the VirtualAccount https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L51-L53 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L56-L58 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L61-L63 but the payableCall function does not have any modifier to protect it, which means that anyone can call this function on any VirtualAccount with any arbitrary data. To give you 2 important examples on how this can be abused :

• anyone can make an arbitrary call to any ERC20 contract, that the VirtualAccount holds, to call the transfer function with one of their addresses as the arguments, stealing all of the ERC20 from the contract
• anyone can make an arbitrary call to any ERC721, since it is stated that VirtualAccount should hold NFT's from platforms like UniswapV3, to transfer the NFT to themselves, which will lead to loss of funds/assets to the owners of VirtualAccount

These are only two examples, but it can be used in multiple ways to hurt the users, the fact that the function is payable does not change anything, since calls can be made with 0 wei, which will basically make the function act like the normal call.

Tools Used

Manual review

Recommended Mitigation Steps

Since you are using the modifier to any other function on the VirtualAccount.sol use it on payableCall also, since calling that function with 0 value, will act the same as normal call and you already have that protected by the modifier.

Assessed type

Access Control