4

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:

  1. What exactly is the purpose of this modifier?
  2. Why is it used only in the trasnsfer function?
  3. Why is it used with a specific size of 64 (2 * 32)?
  4. Why is this modifier present only in a test contract, and not in the actual MultiSigWallet contract? 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 the MultiSigWallet contract? If I don't have any such contract with a standard transfer method (i.e., an ERC20 contract), can I rest assure that I don't need this modifier anywhere else in my system?

Thank you!

4

Questions 1 is answered in this question. There is also much more details at Golem's blog here.

  1. The only reason it was only used in the transfer function is because the contract is for testing. In the wild, this contract would have the safeguard in the transferFrom and approve functions as well.

  2. The 2 * 32 is because that's how large the payload should be for that function. ABI encoding for function parameters has address and uint256 using 1 256 bit slot each, hence why they're calling the modifier with 2 * 32.

  3. This vulnerability is a protection for the person calling the contract against someone that gives them the data. If the person calling the function validates the data is the correct size before submitting it to the network, then they wont have any issues. The large libraries (Web3.js, Ethers.js, etc.) do all this for you when you encode data, e.g. if you use web3.eth.abi.encodeParameters(["address"], ["0x72abde8f"]), it'll throw an error because the address is too short.

Edit: As Smarx has pointed out in the comments, there is a lot of discussion about specifically avoiding this type of fix, as it makes some things even more difficulty as outlined here. Gnosis specifically points this out in the code quoted by OP as well. In the end, my opinion is that protections such as onlyPayloadSize are much better suited to be done off-chain.

7
  • 1
    Note that as one of the answers on that question points out, the current guidance is to not use this type of check.
    – user19510
    Nov 29 '18 at 14:46
  • And it is done off-chain so long as I use web3, right? Can someone attack my contract (the one protected with a MultiSigWallet instance), by passing corrupted data to it without using web3? I mean, the fact that I interact with the contract via web3 makes the onlyPayloadSize redundant. 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)? Nov 29 '18 at 15:01
  • It's very hard to say without specific examples. You really have to think about what would happen if someone sent a short parameter to one of the functions.
    – natewelch_
    Nov 29 '18 at 15:04
  • So you're saying that the MultiSigWallet contract is not really secure by itself, and I would need to check payload size at the beginning of every function intended to be invoked via MultiSigWallet? Nov 29 '18 at 15:07
  • It's not the MultiSigWallet'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 the onlyPayloadSize check anyway
    – natewelch_
    Nov 29 '18 at 15:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.