Incorrect revert message would create confusion leading to negative user experience

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xfb2159138858898a82dcb70c528f07626ebe28c3297cab7e512dc17d0bf81bd0 Severity: minor

Description: Description\ The contest rules for minor severity includes,

Attacks not mentioned in higher categories, but having a negative impact on user experience, or putting one of the contracts in an unexpected state.

This issue is submitted as minor severity and the issue falls in this category.

Most.sol has used custom errors at most of the places in contract functions. This is done to acheive some gas saving as custom errors saves gas as compared to long string message.

Therefore, it is very important to revert message which should be meaningful as well the user should understand why the transaction is reverted with clear revert message. However, i find below custom errors are not clear to users when these will be used as revert message as it will create confusion to user about transaction being successful instead of some message.

    error WrappingEth();
    error UnwrappingEth();
    error EthTransfer();

Let's see, how they are implemented in Most functions,

179        (bool success, ) = wethAddress.call{value: amount}(
180            abi.encodeCall(IWETH9.deposit, ())
181        );
182
183        if (!success) revert WrappingEth();    . . . @audit // revert message is not clear to user or rather creates confusion

249                (bool unwrapSuccess, ) = wethAddress.call(
250                    abi.encodeCall(IWETH9.withdraw, (amount))
251                );
252                if (!unwrapSuccess) revert UnwrappingEth();   . . .  @audit // revert message is not clear to user or rather creates confusion

249                (bool unwrapSuccess, ) = wethAddress.call(
250                    abi.encodeCall(IWETH9.withdraw, (amount))
251                );
252                if (!unwrapSuccess) revert UnwrappingEth();     . . .  @audit // revert message is not clear to user or rather creates confusion

296        (bool success, ) = to.call{value: amount, gas: GAS_LIMIT}("");
297        if (!success) revert EthTransfer()      . . .  @audit // revert message is not clear to user or rather creates confusion

In all above instances, the revert message is not clear, it does not indicate whether the transaction is reverted due to successful or due to some issue.

POC

When the transaction failed with any of the above issue instances then the message like EthTransfer() does not let user to get its true meaning, the user might understand it as ETH is transferred, however the transaction is reverted due to failed ETH transfer.

Therefore, it is important to use clear and meaningful revert message so that user understand why exactly the transaction is failed.

Recommendedation to fix\ Consider below changes so that user experience while using contracts functionality should not be affected negatively.

-    error WrappingEth();
+    error WrappingEthFailed();
-    error UnwrappingEth();
+    error UnwrappingEthFailed();
-    error EthTransfer();
+    error EthTransferFailed();

179        (bool success, ) = wethAddress.call{value: amount}(
180            abi.encodeCall(IWETH9.deposit, ())
181        );
182
-183        if (!success) revert WrappingEth();    
+183        if (!success) revert WrappingEthFailed();   

249                (bool unwrapSuccess, ) = wethAddress.call(
250                    abi.encodeCall(IWETH9.withdraw, (amount))
251                );
-252                if (!unwrapSuccess) revert UnwrappingEth();   
+252                if (!unwrapSuccess) revert WrappingEthFailed();  

249                (bool unwrapSuccess, ) = wethAddress.call(
250                    abi.encodeCall(IWETH9.withdraw, (amount))
251                );
-252                if (!unwrapSuccess) revert UnwrappingEth();     
+252                if (!unwrapSuccess) revert UnwrappingEthFailed();     

296        (bool success, ) = to.call{value: amount, gas: GAS_LIMIT}("");
-297        if (!success) revert EthTransfer()   
+297        if (!success) revert EthTransferFailed()         

[krzysztofziobro]

Not a vulnerability