`AtomWallet.execute()` should be `payable`

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xc83076d185673fc7adc2ae0e837defccab7b394a2652dc20037633a48b506cc8 Severity: medium

Description: In AtomWallet.execute() contract, execute() is used to execute a transaction which is either called from owner or entryPoint.

    function execute(address dest, uint256 value, bytes calldata func) external onlyOwnerOrEntryPoint {
@>      _call(dest, value, func);         @audit // value is non-zero means ether is expected to be send along with function
    }

execute() function takes value as a param and part of transaction call. The value is the ethers which are sent along with function call. execute() calls internal function _call() for the transactions execution which is implemented as below:

    function _call(address target, uint256 value, bytes memory data) internal {
@>      (bool success, bytes memory result) = target.call{value: value}(data);
        if (!success) {
            assembly {
                revert(add(result, 32), mload(result))
            }
        }
    }

It can be seen at (@), the ether value is indeed a part of execute() function as the value is not hardcoded to 0.

Now, the issue is that, execute() will revert when msg.value > 0. The current implementation of the execute() function within the smart contract lacks the payable keyword. This omission leads to a critical issue where any transaction that attempts to send ether (ETH) to this function or with call of this function will fail.

Since the function is designed to allow the owner to execute transaction calls and potentially send ETH, the inability to accept ETH due to the missing payable specifier means that:

1) The contract does not behave as intended when interacting with functions or operations requiring ETH transfers via execute() 2) Any attempt to send ETH to execute() function will revert and result in a failure of the intended operation. 3) ETH sent to this non-payable function will be stuck and effectively lost, leading to financial losses for the function callers.

Impact Details\ execute() will fail when value is greater than 0 so execute() can not be succesfully execute the transaction due to this issue.

Recommendation to fix\ Add payable to execute() function.

Consider below changes:

-    function execute(address dest, uint256 value, bytes calldata func) external onlyOwnerOrEntryPoint {
+    function execute(address dest, uint256 value, bytes calldata func) external payable onlyOwnerOrEntryPoint {
        _call(dest, value, func);
    }

[mihailo-maksa]

The reported issue regarding the missing payable keyword in the execute() function of AtomWallet has been reviewed. Here is our detailed perspective:

Functionality Explanation: The execute function is designed to perform transactions that may include sending Ether (value). The current implementation lacks the payable keyword, which means any attempt to send Ether along with the function call will fail.

Impact Assessment: While this issue does prevent the execute function from handling Ether transfers as intended, it does not result in a security vulnerability or the loss of funds. The function will simply revert if an attempt is made to send Ether, causing a failed transaction rather than a loss.

Severity Adjustment: Given that the issue does not lead to any security exploits or loss of funds but rather affects user experience by causing failed transactions, we consider this issue to be of low severity. It is more of an enhancement to ensure the function behaves as intended when handling Ether transfers.

User Experience: The absence of the payable keyword results in a poor user experience, as users might expect the function to handle Ether transfers seamlessly. Addressing this issue would improve the usability of the function.

Conclusion: The current implementation of the execute function should indeed include the payable keyword to allow Ether transfers. However, since this issue does not lead to any security risks or loss of funds, we consider it to be a low severity issue. It is recommended to add the payable keyword to improve functionality and user experience.

[0xRizwan]

Hi @mihailo-maksa

Thanks for the detailed review.

This should be Medium severity issue based on contest rules for Medium severity:

Attacks that make essential functionality of the contracts temporarily unusable or inaccessible.

This is indeed not a high severity issues which is mostly deals with loss of funds and this issue is originally submitted as Medium severity only. Since one of the functionality is broken and would always revert as described in above issue and it further nicely explained in your detailed comment. This issue should be considered Medium severity based on my above arguments.

[mihailo-maksa]

Thank you for your detailed review and feedback.

After thorough consideration, we have concluded that this issue does not lead to any security exploits or loss of funds but rather affects user experience by causing failed transactions. While we understand your perspective and the importance of ensuring that the function behaves as intended, we believe this issue fits the following category:

"Issues where the behavior of the contracts differs from the intended behavior (as described in the docs and by common sense), but no funds are at risk."

This description aligns almost perfectly with the current situation. The issue does not harm users directly but imposes a limitation on the functionality of the wallet. Therefore, in our opinion, this is correctly categorized as a low severity issue.

Thank you for your understanding.