ERC20 _update customization is not practical. about openzeppelin-contracts HOT 17 CLOSED

Indeed, thanks again.

Ran 1 test for test/HelloWorld.t.sol:OverCapTest
[PASS] testHelloOverCap() (gas: 19073)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 920.50µs (139.40µs CPU time)

Ran 1 test for test/WorldHello.t.sol:OverCapAfterTest
[PASS] testWorldOverCap() (gas: 19065)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 807.10µs (117.40µs CPU time)

from openzeppelin-contracts.

Another example is the capped implementation:

function _update(address from, address to, uint256 value) internal virtual override {
    super._update(from, to, value);

    if (from == address(0)) {
        uint256 maxSupply = cap();
        uint256 supply = totalSupply();
        if (supply > maxSupply) {
            revert ERC20ExceededCap(supply, maxSupply);
        }
    }
}

This could be implemented slightly more efficiently, by being located before the super in this instance, but also by being able to add this check before the total supply increment.

from openzeppelin-contracts.

from openzeppelin-contracts.

Hi @BillSchumacher,

Let's say I want to customize the burn to increment the balance of the 0 adresss and decrement the total supply, It's not currently possible.

Can you elaborate what would be the problem with the following approach?

abstract contract MyERC20 is ERC20 {
    uint256 addressZeroBalance;

    function balanceOf(address account) public view virtual returns (uint256) {
        if(account === address(0)) return addressZeroBalance;
        return _balances[account];
    }

    function _update(address from, address to, uint256 value) internal virtual override {
        super._update(from, to, value);

        if (to == address(0)) {
            addressZeroBalance++;
        }
    }
}

The variables are private intentionally so users DO NOT BREAK the invariants the contract assumes.

This could be implemented slightly more efficiently, by being located before the super in this instance, but also by being able to add this check before the total supply increment.

Could you elaborate on how it's more efficient? At most you'll be saving one if statement, which is about a couple of gas units (JUMPI costs 10 units). The contract is optimized to avoid the most possible SLOADs, which is one of the most expensive operations.

from openzeppelin-contracts.

Hi @BillSchumacher,

Let's say I want to customize the burn to increment the balance of the 0 adresss and decrement the total supply, It's not currently possible.

Can you elaborate what would be the problem with the following approach?

abstract contract MyERC20 is ERC20 {
    uint256 addressZeroBalance;

    function balanceOf(address account) public view virtual returns (uint256) {
        if(account === address(0)) return addressZeroBalance;
        return _balances[account];
    }

This is broken because _balances is private and unavailable. A super to balanceOf does workaround that though.

function _update(address from, address to, uint256 value) internal virtual override {
    super._update(from, to, value);

    if (to == address(0)) {
        addressZeroBalance++;
    }
}

}

The variables are private intentionally so users DO NOT BREAK the invariants the contract assumes.

> This could be implemented slightly more efficiently, by being located before the super in this instance, but also by being able to add this check before the total supply increment.

Could you elaborate on how it's more efficient? At most you'll be saving one `if` statement, which is about a couple of gas units (JUMPI costs 10 units). The contract is optimized to avoid the most possible SLOADs, which is one of the most expensive operations.

In the scenario that we are at the cap two state writes are avoided saving quite a bit of gas, as displayed in the gas remaining screenshots.

With the suggested workaround fixed:

~10000 more gas than required.

Anyhow, I can submit a PR to fix capped if you like with the addition of course so we don't go over the cap before capping,

I appreciate the insight.

from openzeppelin-contracts.

This is broken because _balances is private and unavailable. A super to balanceOf does workaround that though.

Right, this is what I meant:

abstract contract MyERC20 is ERC20 {
  uint256 addressZeroBalance;
  
  function balanceOf(address account) public view virtual returns (uint256) {
      if(account === address(0)) return addressZeroBalance;
      return super.balanceOf(account);
  }
  
  ...
}

A super to balanceOf does workaround that though.

I assume should be fine now.

~10000 more gas than required.

This is straight false, you're measuring the gas just before executing the _update function, which of course will change dramatically the gas consumption.

Please, share a detailed gas comparison using either Foundry or the Hardhat gas reporter for the whole function execution, including the optimization settings and other stuff.

Without optimizations, I'd expect to save only 1 JUMPI on average for your use case. With optimizations enabled and via-ir I'd expect the difference to be waaay smaller. Note that 10 gas units saved is around 0.003 cents in mainnet at 100 gwei, that's the kind of optimizations we won't implement because it compromises other security guarantees of the contract. Variables are private for a reason.

from openzeppelin-contracts.

Why would you execute the update function if you are going to go over the cap?

from openzeppelin-contracts.

Why would you execute the update function if you are going to go over the cap?

Gas measurement should take the full execution of the function into account. Why would you optimize for reverts if you can avoid it with a gas estimation?

from openzeppelin-contracts.

To avoid the state writes

from openzeppelin-contracts.

There are no state writes if you revert, sir.

from openzeppelin-contracts.

The current implementation of the ERC20 already tracks the balance of the zero address. Also, I prepared a Foundry project with both the current implementation and another custom implementation having the cap check before the super._update(...) call.

You can see both implementations here.

After running the following gas measurement:

forge snapshot --via-ir --optimizer-runs 1000000000 --use 0.8.24 --match-test testTransfer -vvvv

The result is exactly the same for both implementations:

MyERC20CappedModifiedTest:testTransfer() (gas: 10316)
MyERC20CappedTest:testTransfer() (gas: 10316)

Closing

from openzeppelin-contracts.

Actually I forgot the addition, it does save gas.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Capped.sol";
error ERC20ExceededCap(uint256, uint256);


abstract contract MyERC20 is ERC20, ERC20Burnable, ERC20Capped {
    function _update(address from, address to, uint256 value) internal virtual override(ERC20Capped, ERC20) {
        if (from == address(0)) {
            uint256 maxSupply = cap();
            uint256 supply = totalSupply();
            if (supply + value > maxSupply) {
                revert ERC20ExceededCap(supply, maxSupply);
            }
        }
        ERC20._update(from, to, value);
    }
}

abstract contract MyERC20After is ERC20, ERC20Burnable, ERC20Capped {
    function _update(address from, address to, uint256 value) internal virtual override(ERC20Capped, ERC20) {
        ERC20._update(from, to, value);
        if (from == address(0)) {
            uint256 maxSupply = cap();
            uint256 supply = totalSupply();
            if (supply > maxSupply) {
                revert ERC20ExceededCap(supply, maxSupply);
            }
        }
    }
}

contract HelloWorld is MyERC20 {
    constructor() payable MyERC20() ERC20("hello", "wrld") ERC20Capped(42)  {
    }

    function mint(uint256 value) public {
        _mint(msg.sender, value);
    }
}

contract WorldHello is MyERC20After {
    constructor() payable MyERC20After() ERC20("hello", "wrld") ERC20Capped(42)  {
    }

    function mint(uint256 value) public {
        _mint(msg.sender, value);
    }
}

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../contracts/HelloWorld.sol";

contract OverCapTest is Test {
    HelloWorld public helloWorld;
    function setUp() public {
        helloWorld = new HelloWorld();
    }
    function testHelloOverCap() public {
        try helloWorld.mint(1000000 * 10 ** helloWorld.decimals()) {
        } catch {
        }
    }
}

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../contracts/HelloWorld.sol";

contract OverCapAfterTest is Test {
    WorldHello public worldHello;

    function setUp() public {
        worldHello = new WorldHello();
    }
    function testWorldOverCap() public {
        try worldHello.mint(1000000 * 10 ** worldHello.decimals()) {
        } catch {
        }
    }
}

from openzeppelin-contracts.

this output seems super unreliable...

from openzeppelin-contracts.

Is there a setting that I should have a certain way?

from openzeppelin-contracts.

Here's the result with 2 million runs:

from openzeppelin-contracts.

If your test doesn't fail to a revert I don't think you're hitting the condition btw.

from openzeppelin-contracts.

I matched your 1 billion runs, which seems absurd.

from openzeppelin-contracts.