Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: How web3 coerces strings into bytes32 #66

Closed
elenadimitrova opened this issue Nov 13, 2017 · 28 comments
Closed

Discussion: How web3 coerces strings into bytes32 #66

elenadimitrova opened this issue Nov 13, 2017 · 28 comments
Labels
discussion Questions, feedback and general information.

Comments

@elenadimitrova
Copy link

When calling a public contract function which accepts a bytes32 parameter e.g. function createNetwork(bytes32 name) with an Ascii string:

rootNetwork.functions.createNetwork('test');

Error is thrown:
Uncaught (in promise) Error: invalid arrayify value

Other contract function return correctly in this instance, implying wiring of the contract is correct.

@ricmoo
Copy link
Member

ricmoo commented Nov 13, 2017

Heya!

This is correct behaviour. An ASCII string is an arbitrary length data type of at least 32 bytes (length followed by packed data) which contains UTF-8.

A bytes32 is fixed length and requires binary data.

Solidity and the block chains are strongly typed, so there is not way to coerce these two types into each other, even manually.

What are you trying to do? You probably want to use a string type instead of a bytes32.

@ricmoo ricmoo added the discussion Questions, feedback and general information. label Nov 13, 2017
@elenadimitrova
Copy link
Author

The bytes32 input is used as a mapping key. Strings shorter than 32 bytes should just translate to left-justified ascii e.g. test => 0x754b00317465737400000000000000000000000000000000000000000000000000000000

The code has been running with truffle-contract and web3 without any issues. Just investigating the difference.

@ricmoo
Copy link
Member

ricmoo commented Nov 13, 2017

That is incredibly strange to me. The decoded string is uK\x001test, which does not seem to even encode the length anywhere. Are you able to decode these?

It may be a bug in web3, but I'm not certain. I will ask Christian what sort of ABI encoded value this is; it may be part of the compact representation.

But in general, it is better to be explicit with what values are intended. If this is expected behaviour, I will likely add a conversion function to convert strings into this representation.

Thanks, and I will keep this thread updated with my findings.

@ricmoo ricmoo changed the title Calling a bytes32 function parameter with an ascii string throws Investigate: How does web3 encode short strings and should ethers.js do something similar Nov 14, 2017
@ricmoo ricmoo added investigate Under investigation and may be a bug. and removed discussion Questions, feedback and general information. labels Nov 14, 2017
@bergutman
Copy link

I'm encountering the same issue with bytes32 parameters. For example the following Solidity function:

function registerHandle(bytes32 handle, bytes name, address contractAddress, bytes avatar, bytes description) {
...
}

Called with:

let sendPromise = contract.registerHandle('connor', 'Connor Gutman', 'exampleaddress', 'photo.png', 'Bla bla');

sendPromise.then(function(transaction) {
   console.log(transaction);
});

Returns the following rejection:

Uncaught (in promise) Error: invalid arrayify value

Converting 'connor' to hex with web3.fromAscii('connor') (Not sure if ethers.js has a function for this) returns 0x636f6e6e6f72 and doesn't reject the promise though! 🎉

@ricmoo
Copy link
Member

ricmoo commented Nov 14, 2017

So, I have confirmed with the author of Solidity that this is at least strange behaviour and likely a bug in web3.

He believes the initial 4 bytes of the string are a selector hash of something, but I haven't been able to figure out of what yet. I was hoping it was just createNetwork(bytes32), but it is something else that will probably require delving into the web3 coder.

This is not behaviour that will be supported in ethers.js, but once I've figured out what it is doing, I can create a sample function to help people who are already relying on the web3 bug.

@ConnorGutman Is there any reason you are not using the solidity string type, if you wish to use strings as the input? Here is how you should be calling your methods if you wish to continue using bytes:

let sendPromise = contract.registerHandle(
    someBytes32 // This one cannot by done until I figure out what strange transform web does: 'connor',
    ethers.utils.toUtf8Bytes('Connor Gutman'),
    '0x0123456789012345678901234567890123456789', // Must be a valid address
    ethers.utils.toUtf8Bytes('photo.png'),
    ethers.utils.toUtf8Bytes('Bla bla')
);

It is important to note there is a very significant difference in general between a string and a bytes. A string of length 8 is not necessarily a bytes of length 8.

I would recommend if this is the way you wish to use the contract you change the signature to:

function registerHandle(bytes32 handle, string name, address contractAddress, string avatar, string description) {

Make sense?

@ricmoo
Copy link
Member

ricmoo commented Nov 14, 2017

Oh! The string you sent is the full bytes sent to the network, including the function selector.

To mimic the web3 coercion of short string into bytes32 you can use the following; but please be aware you are relying on incorrect behaviour, so terrible things will happen, if for example the string is over 32 bytes (not necessarily characters) long:

function web3StringToBytes32(text) {
    var result = ethers.utils.hexlify(ethers.utils.toUtf8Bytes(text));
    while (result.length < 66) { result += '0'; }
    if (result.length !== 66) { throw new Error("invalid web3 implicit bytes32"); }
    return result;
}

@ricmoo ricmoo changed the title Investigate: How does web3 encode short strings and should ethers.js do something similar Discussion: How web3 coerces strings into bytes32 Nov 14, 2017
@ricmoo
Copy link
Member

ricmoo commented Nov 14, 2017

To give a quick example of how the web3 behaviour can cause your entire application to behave in wildly unpredictable ways (possibly exposing it to attacks); imagine if you accept user input from a web form which is a string:

mainnet> web3Coder = require('web3/lib/solidity/coder');

// The string "test" is 4 bytes long:
mainnet> web3Coder.encodeParam('bytes32', 'test')
'7465737400000000000000000000000000000000000000000000000000000000'

// The string "0x12" is 1 byte long
mainnet> web3Coder.encodeParam('bytes32', '0x12')
'1200000000000000000000000000000000000000000000000000000000000000'

@bergutman
Copy link

@ricmoo thanks! I was actually using bytes over strings as a very temporary workaround for a pesky problem with web3.js. It seems that this issue doesn't arise when using ethers.js though so I've gone ahead and updated my smart contract to use strings!

Essentially react native has a utility that takes precedence over utf8 (mathiasbynens/utf8.js#17) which web3 requires for calls such as utf8.encode() and utf8.decode() when dealing with strings. Since Facebook seems to have no interest in changing this, you either have to fork web3.js or build some crazy post-install script to change all the calls for utf8 😬. I just decided to table it until things were further along with my app. Ethers.js seems to have sorted it out for me though so cheers! 🍻

@ricmoo
Copy link
Member

ricmoo commented Nov 14, 2017

@ConnorGutman Awesome! Glad to here it! Yes, to convert between strings and bytes in ethers, you can use:

var text = "hello world";
var bytes = ethers.utils.toUtf8Bytes(text);
var textAgain = ethers.utils.toUtf8String(bytes);

Thanks!

@ricmoo ricmoo added discussion Questions, feedback and general information. and removed investigate Under investigation and may be a bug. labels Nov 17, 2017
@ricmoo
Copy link
Member

ricmoo commented Nov 17, 2017

Does the above solution using web3StringToBytes32 work for everyone?

If so, I'll close this.

@elenadimitrova
Copy link
Author

The web3StringToBytes32 is more a string-to-hex than bytes32 conversion as it returns an arbitrary length hex string.

From my part we can close this as I've gone up to v1 of web3.utils which has a utf8ToHex, asciiToHex and a handful of other useful conversion functions http://web3js.readthedocs.io/en/1.0/web3-utils.html?highlight=toUTF#utf8tohex

Also, the auto conversion between strings (used as keys) and their encoded bytes32 hex equivalent has been discussed here well before the v1 release above web3/web3.js#271 including comments from "the author of Solidity" :)

@ricmoo
Copy link
Member

ricmoo commented Nov 19, 2017

The above web3StringToBytes32 will always return a string of length 66 (which is the 0x prefix followed by 64 nibbles; which represents. single bytes32).

> web3StringToBytes32('test')
'0x7465737400000000000000000000000000000000000000000000000000000000'

Which is what you would need to pass into any legacy function which takes a bytes32 in as input, but was being passed a string through web3, then coerced into bytes32.

Yes, the equivalent functions in ethers are ethers.utils.toUtf8String and ethers.utils.toUtf8Bytes.

Exactly, Christian brings up the exact problems with using strings as keys, which is why the method he mentioned is what Solidity internally uses (after constructing the intermediate hash) to create the mapping. If you use provider.getStorageAt this becomes more relevant, but luckily the solidity language handles most of this for you. :)

I'll close this, but please feel free to re-open or start a new discussion on features you would like to see, or discuss this further. I'm need to make a "migrating from web3 to ethers.js" document sometime, and I will certainly add this.

Thanks!

@ricmoo ricmoo closed this as completed Nov 19, 2017
@elenadimitrova
Copy link
Author

elenadimitrova commented Nov 20, 2017

Just to be clear, the above web3StringToBytes32 returns a minimum of 32 bytes but it has no maximum so web3StringToBytes32('test1test1test1test1test1test1test1test1test1test1'); for example returns 102 bytes 0x7465737431746573743174657374317465737431746573743174657374317465737431746573743174657374317465737431

@ricmoo
Copy link
Member

ricmoo commented Nov 20, 2017

The intention of the function though is to take in a "short string", which was once used with the web3 coder object which was implicitly converted to a bytes32.

You bring up a good point though. What does web3 do when you pass in a 33 byte string to a function that accepts a bytes32? I can make the function emulate that behaviour. I would guess it is one of:

  • truncation (this would be terrible and frightening)
  • completely corrupt the encoded data (also, terrifying :))
  • throws an error

I will update the above function to throw an error for now, but please let me know what web3 does.

@skylerisskyler
Copy link

I am using the Simple Store contract Truffle box to send a Javascript string to my contract. But I always get another promise error.
What I have tried:

  • Changed all the variables to bytes32 and bytes32[] in simplestore.sol
  • Used methods web3.fromAscii and ethers.toUft8Bytes

I also had to install web3-utils separate from web3 and downgrade web3 to avoid errors.

Maybe I'm going at this all the wrong way. My greater project is to have a user input a string on the front-end that can be hashed with keccak256 then keyed to a value in a mapping in my contract.

@ricmoo
Copy link
Member

ricmoo commented Feb 2, 2018

@rileyskyler

Can you provide the source code you are using? You should not need any Web3 libraries.

Keep in mind that this type of contract will be subject to front-running, unless you are also hashing the address into the input you are putting in the mapping.

@skylerisskyler
Copy link

skylerisskyler commented Feb 2, 2018

Sure @ricmoo ,
App.js:

simpleStorage.deployed().then((instance) => {
         simpleStorageInstance = instance
            return simpleStorageInstance.set(ethers.utils.toUtf8String("test"), {from: accounts[0]})
     }).then((result) => {
         console.log(result)

SimpleStorage.sol

contract SimpleStorage {
  bytes32 storedData;

  function set(bytes32 x) public {
    storedData = x;
  }

  function get() public view returns (bytes32) {
    return storedData;
  }
}

This is a piece of what I want to implement in my dApp, runs as anticipated on Remix:

    struct hourToken {
       bool unredeemed;
    }
    
    mapping (bytes32 => hourToken) public hourTokens;
    
    function createToken(string _str) public onlyOwner {
        bytes32 hashedStr = keccak256(_str);
        hourTokens[hashedStr].unredeemed = true;
    }

    function redeem(string _str) public returns (bool) {
        bytes32 hashedStr = keccak256(_str);
        require(hourTokens[hashedStr.unredeemed == true);
        hourTokens[hashedStr].unredeemed = false;
        return hourTokens[hashedStr].unredeemed;
    }

I'm not sure what you mean by front-running.

@ricmoo
Copy link
Member

ricmoo commented Feb 2, 2018

I don't quite understand the syntax, looks like the Truffle part is adding a bunch of qualifiers, but I will comment as best I can. :)

Your contract input is a bytes32, so you need to pass in a bytes32. Also, strings in JavaScript are already UTF-8 strings, so you do not need the toUtf8String. What you probably mean to do is something like:

return simpleStorageInstance.set(ethers.utils.keccak256(ethers.utils.toUtf8Bytes("test")))
// Or you can use the built-in function that does both these operations, `id`
// return simpleStorageInstance.set(ethers.utils.id("test"))

Or if you are trying to emulate the "incorrect" behaviour of old Web3 dApps, you can use the function above.

In your new code however, you will pass the string in directly to createToken, no need to wrap it at all, with contract.createToken("test").

Does that make sense? I don't fully understand what Truffle is doing, with the "Instance" stuff, possibly using the Web3 Contract object, which is quite different from an ethers Contract object.

@paulbarclay
Copy link

paulbarclay commented Mar 2, 2018

@ricmoo I couldn't find any docs on this, and ran into what looked like this problem, so I tested web3 with longer strings being passed to bytes32, using both Remix, and the latest Truffle version. For Remix, the answer is (1) - it truncates. For Truffle, the answer is (2) - it does not truncate, but instead corrupts the data by appending the whole string as a series of bytes32, pushing the extra calldata out. Tested using a function: createCharacter(bytes32 name, uint stat1, uint stat2). If you call this as createCharacter("my name", 100, 150), in either truffle or remix you get:

name = "my name"
stat1 = 100
stat2 = 150

But if you call it in Truffle as createCharacter("my name is way too long and will be truncated", 100, 150), you get:

name = "my name is way too long and will"
stat1 = gibberish integer
stat2 = 100 (not 150)

You can protect against the corruption by putting the bytes32 field at the end, but nothing protects you from the truncation. And neither tool highlights the core issue - passing a string that doesn't fit into a bytes32 should throw.

@ricmoo
Copy link
Member

ricmoo commented Mar 3, 2018

Yeah, the "correct" thing to do should be to throw, regardless of the string, as these types are not actually compatible. Due to a legacy Web3 bug, this currently sometimes works, but as you pointed out can corrupt things.

I believe ethers.js should throw an exception if you try this, doesn't it?

If you really want to do this, you can create a function like:

function stringToBytes32(text) {
    var data = utils.toUtf8Bytes(text);
    if (data.length > 32) { throw new Error('too long'); }
    data = utils.padZeros(data, 32);
    return utils.hexlify(data);
}

@billtlee
Copy link

Hi,

I am trying to pass byte32 to a contract. When I try to execute the following code I get an invalid arrayify value error. Can someone please help? Here is the solidity function and the javascript call:

    function setValueBytes32(bytes32 value) public {
        _valueBytes32 = value;
    }

    const contract = new ethers.Contract(address, abi, wallet);  

    contract.setValueBytes32('hello world').then(transaction => {
      console.log(transaction)
    })

@billtlee
Copy link

I am just doing some testing with this. I have another contract that is using bytes32. The reason for using bytes32 instead of string is gas costs. Thanks!

@ricmoo
Copy link
Member

ricmoo commented Aug 16, 2018

Hey @billtlee ,

"Hello World" is not a valid bytes32. You must pass in binary data and it must be 32 bytes for a bytes32. You application will need to understand how to deal with longer strings (keep in mind there are potentially differences in the length of the string and the length of the string in bytes; you may be opening your application to certain types of security vulnerabilities by using a bytes32 instead of a string) and how to parse them, but you can use a function similar to:

var Zeros = "0x0000000000000000000000000000000000000000000000000000000000000000";
function stringToBytes32(str) {
    let bytes = ethers.utils.toUtf8Bytes(str);
    if (bytes.length > 31) { throw new Error('too long'); }
    return ethers.utils.concat([bytes, Zeros]).slice(0, 32);
}

I know a lot of people come from Web3 expecting this to "just work", but it enables dapp developers to do very unsafe operations without realizing it. I try in general to make sure if a developer is doing something they (in many cases) shouldn't, that they realize it. :)

I will add safe versions of formatBytes32String and parseBytes32String to the utils, since parse is actually quite complicated. The maximum length for a string should also be 31 bytes, to ensure there is a null-termination.

@billtlee
Copy link

Thanks @ricmoo! Yes, came from web3 haha. I tried to use ethers.utils.toUtf8Bytes also, but couldn't get that to work. Didn't do ethers.utils.concat([bytes, Zeros]).slice(0, 32). Maybe that's why. I will give it a try. It'd be great if we can have formatBytes32String and parseBytes32String in the utils. When might you release that?

@ricmoo
Copy link
Member

ricmoo commented Aug 16, 2018

I'm working on it now. It will only be available in the 4.x versions and able though. npm install ethers@next.

@ricmoo
Copy link
Member

ricmoo commented Aug 16, 2018

There is a lot of thought into security though, so it might take a few days before I convince myself I'm doing this a secure way.

@billtlee
Copy link

Thanks! I opened another issue on this under #66 (comment). Feel free to close that one.

@sugalivijaychari
Copy link

sugalivijaychari commented Mar 25, 2021

My solidity Code:
function execute(bytes32 txHash,uint8[] memory sigV, bytes32[] memory sigR, bytes32[] memory sigS)returns(address[] memory){ address[] public recoveredAddr; for(uint i=0; i<sigV.length; i++){ address recovered = ecrecover(txHash, sigV[i], sigR[i], sigS[i]); recoveredAddr.push(recovered); } return recoveredAddr; }

My test.js code:
Here txHash is same(calculated in both smart contract and node.js script) and pk is private keys of accounts which we recover by invoking execute function.
let sigV=[],sigR=[],sigS=[]; var signature = web3.eth.accounts.sign( txHash , pk ); sigV.push(web3.utils.hexToBytes(signature.v)[0]); sigR.push(signature.r); sigS.push(signature.v); const result = await solidityContract.methods.execute(txHash,sigV, sigR, sigS).send({ from: accounts[0], gas:3000000 }); console.log(result);
Now, result is different with actual accounts.
By researching, I came to know that console.log(typeof(signature.r)) is string but not bytes32.
console.log(typeof(sigR)) is object but not array.

As signature.r or signature.s have length with 66 which is greater than 32. How can we convert 66 characters length of string to bytes32 and pass to execute function to achieve same result i.e, recovered accounts are same to actual accounts where we sign.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Questions, feedback and general information.
Projects
None yet
Development

No branches or pull requests

7 participants