Cfrgtkky

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!

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

2. 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.




3. 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.




4. 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.