OpenZeppelin/openzeppelin-contracts

Improve gas efficiency of `SafeERC20.forceApprove` when the token returns a bool on approve

fedgiac opened this issue · 4 comments

fedgiac commented

🧐 Motivation

More gas-efficient implementation of SafeERC20.forceApprove in the case where the token returns a bool on approve.
Expected savings are a little more than a call to EXTCODESIZE, that is 2600 gas in the worst case of no access lists or 100 gas otherwise.
The required code changes are very small.

📝 Details

forceApprove is the only function calling _callOptionalReturnBool (code).
This function is supposed to behave like _callOptionalReturn except that if the inner call reverts then it returns false instead of reverting.
The output is currently computed as:

success && (returnsEmptyOutput || returnsBoolTrue) && targetHasCode

My proposal it to change this expression to:

success && ((returnsEmptyOutput && targetHasCode) || returnsBoolTrue)

With the proposed change, if returnsEmptyOutput is false then the evalutation of the second && short-circuits and the code is assumed to have code.
This is the same assumption made in _callOptionalReturn through functionCall.

AryanSherigar commented

Hi, if no one is working on this issue i would like to work on this issue

  • 👍1
AryanSherigar commented

So, i did the changes proposed in the issue. And according to me the changes are

function _callOptionalReturnBool(IERC20 token, bytes memory data) private returns (bool) {
    (bool success, bytes memory returndata) = address(token).call(data);
    return success && ((returndata.length == 0 && address(token).code.length > 0) || abi.decode(returndata, (bool)));
}

and after doing this changes while running the test I got an error

  1. SafeERC20
    with address that has no contract code
    reverts on forceApprove:
    AssertionError: Expected transaction to be reverted with custom error 'AddressEmptyCode', but it reverted without a reason
    at Context. (test\token\ERC20\utils\SafeERC20.test.js:80:7)

So, am i require to change the forceApprove function so that it first check if the Address is empty or not or everything is correct i have to modify the unit test for this

AryanSherigar commented

I guess the changes done in pull request #5090 will address this issue too

ernestognw commented

I guess the changes done in pull request #5090 will address this issue too

Hey @AryanSherigar, yes, it seems like that's the case since we're using a ternary:

returnSize == 0 ? address(token).code.length > 0 : returnValue == 1

We already merged that PR, sorry for not linking to this issue before

  • 👍1