In a contract used for testing Gnosis MultiSigWallet, I found this piece of code:
/*
* This modifier is present in some real world token contracts, and due to a solidity
* bug it was not compatible with multisig wallets
*/
modifier onlyPayloadSize(uint size) {
require(msg.data.length == size + 4);
_;
}
/// @dev Transfers sender's tokens to a given address. Returns success.
/// @param _to Address of token receiver.
/// @param _value Number of tokens to transfer.
/// @return Returns success of function call.
function transfer(address _to, uint256 _value) onlyPayloadSize(2 * 32)
public
returns (bool success)
{
require(balances[msg.sender] >= _value);
balances[msg.sender] -= _value;
balances[_to] += _value;
Transfer(msg.sender, _to, _value);
return true;
}
The documentation of the onlyPayloadSize modifier is extremely unclear to me.
Can somebody please explain the following:
- What exactly is the purpose of this modifier?
- Why is it used only in the
trasnsferfunction? - Why is it used with a specific size of 64 (
2 * 32)? - Why is this modifier present only in a test contract, and not in the actual
MultiSigWalletcontract? Does this by any chance imply that I should add this modifier in every contract in my system, designated for access only via an instance of theMultiSigWalletcontract? If I don't have any such contract with a standardtransfermethod (i.e., an ERC20 contract), can I rest assure that I don't need this modifier anywhere else in my system?
Thank you!
MultiSigWalletinstance), by passing corrupted data to it without using web3? I mean, the fact that I interact with the contract via web3 makes theonlyPayloadSizeredundant. But can someone use the fact that I have omitted this modifier, and attack my contract by interracting with it directly (i.e., not via web3)? – goodvibration Nov 29 '18 at 15:01MultiSigWalletcontract is not really secure by itself, and I would need to check payload size at the beginning of every function intended to be invoked viaMultiSigWallet? – goodvibration Nov 29 '18 at 15:07MultiSigWallet's responsibility to validate the data being passed to your contract. It can't be insecure at something it has no knowledge, context, or responsibility about. Its responsibility is to only act on transactions that have been given proper authorization by whatever its x-of-x scheme, and it's likely pretty safe at doing that. You need to analyze your own contracts (or pay someone else to) outside the scope of the multisig to be sure they're safe. Note in the edit that many contracts (including Gnosis safe) can't call contracts with theonlyPayloadSizecheck anyway – natewelch_ Nov 29 '18 at 15:13msg.data.lengthmust be verified prior to everything else? – goodvibration Nov 29 '18 at 15:24