msg.value was not handled properly

[Beosin20180329]

The executeMetaTransaction function has the payable modifier, and users can pass in native tokens such as ETH when calling this function. But the incoming ETH is not handled properly inside executeMetaTransaction.

There is no msg.value passed in the function call here:
https://github.com/bcnmy/metatx-standard/blob/master/src/contracts/EIP712MetaTransaction.sol#L49

If the target function relies on msg.value for data recording, it may cause that the ETH passed in by tx.origin cannot be processed or the target function throws an exception. The native token will remain in the contract and may not be able to be withdrawn.

The test contract looks like this:

contract Pool is EIP712MetaTransaction("TestMetaTransaction", "1"){
    mapping (address=>uint256) public userInfo;
    function deposit(uint256 amount) public payable{
        require(msg.value==amount);
        userInfo[msgSender()] = userInfo[msgSender()] + msg.value; 
    }
    function withdraw(uint256 amount)public {
        payable(msgSender()).transfer(amount);
        userInfo[msgSender()] = userInfo[msgSender()] - amount;
    }
}

When the executeMetaTransaction function is used to stake for other addresses, ETH is sent to the Pool contract, but it cannot be recorded in the userInfo mapping. As a result, the transfer cannot be withdrawn.

It is suggested to change to:

(bool success, bytes memory returnData) = address(this).call{value:msg.value}(abi.encodePacked(functionSignature, userAddress));

BEOSIN is a leading global blockchain security company dedicated to the construction of blockchain security ecology.

If you need additional assistance, please contact us from:
twitter: https://twitter.com/Beosin_com
telegram: https://t.me/dawnxia

[livingrockrises]

Hello,
executeMetaTransaction is meant to be called by Biconomy relayer EOA with information of user's signature and function signature that user/devs intend to be gasless(non payable)

Given above, additionally relayers will always send msg.value = 0 hence meta transactions are not supported for methods like deposit when there is value transfer of native currency involved. We suggest devs to take deposits in ERC20 Tokens and convert them to native currency, if possible.

I will be removing payable on this method as it doesn't make sense and not really required.
Thank you! Feel free to share your thoughts

[Beosin20180329]

OK. Since some projects use executeMetaTransaction to call functions that require msg.value, and because executeMetaTransaction can accept ETH transfers, we think that maybe the executeMetaTransaction function forgot to internally process msg.value.

The presence of the payable modifier may have led the developer to believe that the function requiring msg.value could be called from the executeMetaTransaction function.

In fact, if Alice does not have any ETH, but needs to call a function with the payble modifier, a direct call through Bob can save the handling fee of an ETH transfer instead of through Bob -> Alice -> Contract.

[livingrockrises]

Got you. When preparing raw transaction in the backend if a relayer just reads msg.value and passes on then it could be exploited. In above case Bob needs assurance that Alice will be charged (directly or indirectly sponsored off by someone) pre or post relay call in any form as meta transactions are not free.

[sebastiantf]

When intending to use with some relay services apart from Biconomy, (eg. The Box) which does allow ether value transfer via msg.value, it could make sense for the executeMetaTransaction() to pass along the ether.

Wondering if thats okay / safe to modify the standard contract for that purpose. The responsibility of ensuring the ether transfer is safe and accounted rests on the relay service I suppose.

[livingrockrises]

we can modify the contract to pass on the value.