Inconsistent library address validations between CLI and Standard JSON

[cameel]

Checks applied to the value of --library option in CLI are not exactly the same as the ones for libraries in Standard JSON. For example only CLI checks address checksum. The validations should be reviewed and unified.

[aaroosh-07]

@cameel i want to take up this issue. I have builded up the solidity compiler on in my local environment and have successfully run all the test.
Please guide me how can i solve this issue.

[cameel]

Sure. The problem is that checks in this part of CommandLineInterface::parseLibraryOption():

solidity/solc/CommandLineInterface.cpp

Lines 678 to 748 in 09578e7

	for (string const& lib: libraries)
	if (!lib.empty())
	{
	//search for equal sign or last colon in string as our binaries output placeholders in the form of file=Name or file:Name
	//so we need to search for `=` or `:` in the string
	auto separator = lib.rfind('=');
	bool isSeparatorEqualSign = true;
	if (separator == string::npos)
	{
	separator = lib.rfind(':');
	if (separator == string::npos)
	{
	serr() << "Equal sign separator missing in library address specifier \"" << lib << "\"" << endl;
	return false;
	}
	else
	isSeparatorEqualSign = false; // separator is colon
	}
	else
	if (lib.rfind('=') != lib.find('='))
	{
	serr() << "Only one equal sign \"=\" is allowed in the address string \"" << lib << "\"." << endl;
	return false;
	}
	string libName(lib.begin(), lib.begin() + static_cast<ptrdiff_t>(separator));
	boost::trim(libName);
	if (m_libraries.count(libName))
	{
	serr() << "Address specified more than once for library \"" << libName << "\"." << endl;
	return false;
	}
	string addrString(lib.begin() + static_cast<ptrdiff_t>(separator) + 1, lib.end());
	boost::trim(addrString);
	if (addrString.empty())
	{
	serr() << "Empty address provided for library \"" << libName << "\"." << endl;
	serr() << "Note that there should not be any whitespace after the " << (isSeparatorEqualSign ? "equal sign" : "colon") << "." << endl;
	return false;
	}
	if (addrString.substr(0, 2) == "0x")
	addrString = addrString.substr(2);
	else
	{
	serr() << "The address " << addrString << " is not prefixed with \"0x\"." << endl;
	serr() << "Note that the address must be prefixed with \"0x\"." << endl;
	return false;
	}
	if (addrString.length() != 40)
	{
	serr() << "Invalid length for address for library \"" << libName << "\": " << addrString.length() << " instead of 40 characters." << endl;
	return false;
	}
	if (!passesAddressChecksum(addrString, false))
	{
	serr() << "Invalid checksum on address for library \"" << libName << "\": " << addrString << endl;
	serr() << "The correct checksum is " << getChecksummedAddress(addrString) << endl;
	return false;
	}
	bytes binAddr = fromHex(addrString);
	h160 address(binAddr, h160::AlignRight);
	if (binAddr.size() > 20 || address == h160())
	{
	serr() << "Invalid address for library \"" << libName << "\": " << addrString << endl;
	return false;
	}
	m_libraries[libName] = address;
	}

and this one in StandardCompiler::parseInput():

solidity/libsolidity/interface/StandardCompiler.cpp

Lines 836 to 871 in 09578e7

	for (auto const& sourceName: jsonLibraries.getMemberNames())
	{
	auto const& jsonSourceName = jsonLibraries[sourceName];
	if (!jsonSourceName.isObject())
	return formatFatalError("JSONError", "Library entry is not a JSON object.");
	for (auto const& library: jsonSourceName.getMemberNames())
	{
	if (!jsonSourceName[library].isString())
	return formatFatalError("JSONError", "Library address must be a string.");
	string address = jsonSourceName[library].asString();
	if (!boost::starts_with(address, "0x"))
	return formatFatalError(
	"JSONError",
	"Library address is not prefixed with \"0x\"."
	);
	if (address.length() != 42)
	return formatFatalError(
	"JSONError",
	"Library address is of invalid length."
	);
	try
	{
	ret.libraries[sourceName + ":" + library] = util::h160(address);
	}
	catch (util::BadHexCharacter const&)
	{
	return formatFatalError(
	"JSONError",
	"Invalid library address (\"" + address + "\") supplied."
	);
	}
	}
	}

are not the same. In particular:

• CLI checks address checksum but Standard JSON does not.
• CLI checks if the address is empty but Standard JSON does not. This check is redundant because we still check address length later but if it's there, it should be in both places.
• The checks for invalid characters are different (one uses fromHex(), the other relies on h160 constructor throwing exceptions). They are probably equivalent but there's really no reason for them not to be identical.

So the idea would be to make these checks as close as possible. I think it would be best to extract the validation into a single function called from both places.

The PR should include at least minimal commandline tests covering these new cases (unless they're already covered - please check). Here are some of the existing tests:

• https://github.com/ethereum/solidity/tree/develop/test/cmdlineTests/linking_standard_solidity/
• https://github.com/ethereum/solidity/tree/develop/test/cmdlineTests/linking_solidity/

There are more in subdirectories starting with linking_.

Note that soltest does not execute these tests. There's a separate script for running them: test/cmdlineTests.sh.

We also have tests covering this code in test/libsolidity/StandardCompiler.cpp but these are more suitable if you actually want to check the results in some more complex way. For just testing the error message command-line tests should be enough.

[cameel]

Also, there's another issue that's related to this: #10298. You might want to take it once you fix this one.

[cameel]

One more thing: there's a pending refactor that's likely to be merged into develop very soon: #11518. It moves the part I mentioned to CommandLineParser.cpp. You might want to start your branch already on top of it if you want to avoid rebasing it later.

[chriseth]

I wonder if we should target the breaking branch for this.

[cameel]

Good point. Do we consider changes like that breaking? Technically it will now no longer be possible to use a library address with invalid checksum in Standard JSON, though we could also consider the lack of this check a bug.

[chriseth]

The thing is that if we require checksums, we require the "caller" to include the full keccak algorithm as a dependency. Does it create an error or a warning?

In any case, @aaroosh-07 don't be discouraged by this discussion, we certainly want the feature, the question is just which version it should land in, but you can already start working on it regardless of that.

[cameel]

Well, solc-js already depends on it (js-sha3) so JS tools have that dependency already unless they go out of their way to use the compiler binary directly. Also looks like web3.js, whch a lot of them use depends on @ethersproject/keccak256. Might be a bigger problem for tools that use CLI with --standard-json (Brownie, dapp-tools) but they still usually let you download the compiler so they likely already have a way to verify keccak checksums (we only added SHA256 for binaries very recently).

Does it create an error or a warning?

It's an error:

 		if (!passesAddressChecksum(addrString, false)) 
 		{ 
 			serr() << "Invalid checksum on address for library \"" << libName << "\": " << addrString << endl; 
 			serr() << "The correct checksum is " << getChecksummedAddress(addrString) << endl; 
 			return false; 
 		}

But it would not be hard to change it to a warning. Might actually be the best solution since it does not introduce any backwards-compatibility issues.

[chriseth]

If the recommended checksum is not too hard to parse, it may be a good solution for a caller that does not have a keccak library to just iterate.

[aaroosh-07]

okay I'm on to this issue @cameel @chriseth

[aaroosh-07]

To solve this issue, I have to add checks which are not present in standard complier but present in commandLineInterface
Am I understanding the issue properly?

[cameel]

Yes.

But also please try to extract the common parts of this code into a function that we can reuse in both places. The function could take the address as a string, verify it's a valid and return it as h160. If it's not valid it should return an error message (either by throwing an exception or as a part of the result - you could use Result<h160> type for that).

[aaroosh-07]

progress till now

1. added test for empty string in standardcompiler.cpp
2. added test for checking address checksum in standardcompiler.cpp

[aaroosh-07]

Had internal examinations past three days because of which had slow progress.

[cameel]

@aaroosh-07 How is it going? Do you need any help with the implementation?

[aaroosh-07]

sorry @cameel because of my university examinations going on, I have made not much progress on this issue.

[aaroosh-07]

the exceptions throwed by constructor h160 to check for address, is being used at multiply places in the file standard compiler.cpp so should I leave it as is?

[cameel]

No worries. Take your time. Just checking if you're still on it or if the task should be marked as available for someone else to take.

the exceptions throwed by constructor h160 to check for address, is being used at multiply places in the file standard compiler.cpp so should I leave it as is?

Either way is ok, just make sure that the same mechanism is used in both places. If you leave exceptions in StandardCompiler.cpp then just use them in CommandLineParser.cpp too.

[imlishant]

Hi everyone,
I am new to coding and open source, how can I contribute here?
I have beginner knowledge of C & C++.

Any help would be highly appreciated.

Thanks & Regards