Hello all! Thank you everyone for those who have joined and contributed to the forum so far, we’re hoping to keep things active here, so I have a few topics I’d like to ask everyone about for some good debates.
Do smart contract developers, especially those using Solidity have a poor habit of over-securing?
Here are some examples:
If you go back to older versions of the OpenZeppelin library, you will see transfer functions that have several requires in place.
// v1.12.0 for example
function transferFrom(address _from, address _to, uint256 _value) public returns (bool) {
require(_value <= balances[_from]);
require(_value <= allowed[_from][msg.sender]);
require(_to != address(0));
balances[_from] = balances[_from].sub(_value);
balances[_to] = balances[_to].add(_value);
allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value);
emit Transfer(_from, _to, _value);
return true;
}
Which has now been reduced to:
// v2.1.2 now
function transferFrom(address from, address to, uint256 value) public returns (bool) {
_allowed[from][msg.sender] = _allowed[from][msg.sender].sub(value);
_transfer(from, to, value);
emit Approval(from, msg.sender, _allowed[from][msg.sender]);
return true;
}
One other common occurrence I see are internal functions that return true with the calling function wrapping the return around a require(). Isn’t it not needed because the internal function will always return true?
There are a few more examples but I wanted to know what everyone’s thought was on this habit. Security is very important and it’s important to make sure we don’t do more than what is actually needed.
Yeah, in more recent versions we removed the extra balance and allowance checks because they were already being handled by SafeMath, and there was concern over the additional gas.
I personally favor redundance, and I think the compiler should work hard to remove redundant operations and optimize the bytecode. But the Solidity compiler is not there yet, it seems, so we're compromising.
Security is very important and it’s important to make sure we don’t do more than what is actually needed.
I'd actually change this to: security is very important, but performance can be important too. The balance has to be chosen carefully, and I think inexperienced developers should be conservative and favor security checks in depth at each module boundary.
Longer answer
There is nothing as over-securing. Writing the same check again in most cases is just bad coding practice. In early times of ethereum, solidity and solidity compiler were new and relatively untested so adding the same check twice in supercritical locations was kinda ok (OZ’s earlier transfer functions). That being said, I believe solc is now fairly well tested and I see no point in adding same checks twice.
Some devs use SafeMath everywhere though. Not because they don’t know that they are wasting gas in an extra check but because they don’t want to forget using it in places where it is actually required. If you always use it, you can’t forget it.
Totally fine to favor redundance, although for OpenZeppelin in particular this is code that is reused multiple times so redundancies that aren't needed cost more gas for everyone who is using the OZ code. Thankfully the OZ codebase is more mature now, the current progress on master actually looks really nice!
Good point about the compiler! If it were better at catching this kind of stuff it would be less of a problem. Do you know anything about their progress on that?
Ahhh I see, that explains a bit more then. The development community itself has also become more mature.
True! And spotting where it's not needed is more difficult than just doing it everywhere and not worrying about the gas. It's a minor extra cost.
From my relatively short experience performing security audits of a wide variety of projects involving Solidity smart contracts, my answer would be: No, but there’s definitely more awareness about security. Even if developers did have a habit of “over-securing”, I’d rather have that over an “under-securing” habit.
I agree that adding lots of redundancy in terms of security may lead to gas-related inefficiencies down the line, so I guess it’s fine to sometimes favor efficiency instead of explicitness and redundancy - as long as these benefits from saving some gas are actually measured via proper testing and do not compromise security in any sense.
@IvanTheGreatDev to add to this, I believe that SafeMath used to use assert rather than require and asserts are meant to check conditions that should never be true, not for enforcing conditions. But with SafeMath now using require the checks that were in transferFrom are redundant.
I personally lean towards optimizing for security and readability rather than gas efficiency like @frangio said which sometimes means redundant checks but I try to strike a good balance based on the use case.
Now, instead of too many requires or something similar. What about the design pattern of:
function doAVeryImportantThing() public {
require(checkThing());
otherStuff...;
}
function checkThing() internal returns (bool) {
doSomeChecks();
return true;
}
I used to see this type of “require internal bool function” very commonly a while back, does doing this add any extra benefits? Was there a reasoning for this?
I dislike that pattern since it is easy to forget to add the require on the callsite. Granted, it allows reacting to errors as opposed to simply reverting, but those errors usually stem from invalid parameters, etc., so there’s actually no reasonable response to the error other than bubbling it up (in a revert).
I generally tend to favor a “security in depth” approach, as a design principle for smart contract development code. I think redundant checks, or more generally implementing independent protections against errors and attacks, help catch problems we didn’t think about when writing the code.
edit: Premature gas optimization can lead to major security holes. In this early stage of the industry, I’d favor correctness over efficiency.
Thank you guys!!