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

Gas Optimizations #31

Open
code423n4 opened this issue Mar 21, 2022 · 4 comments
Open

Gas Optimizations #31

code423n4 opened this issue Mar 21, 2022 · 4 comments
Labels
bug Something isn't working G (Gas Optimization) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S] ERC20.sol#transferFrom() Do not reduce approval on transferFrom if current allowance is type(uint256).max

The Wrapped Ether (WETH) ERC-20 contract has a gas optimization that does not update the allowance if it is the max uint.

The latest version of OpenZeppelin's ERC20 token contract also adopted this optimization.

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/ERC20.sol#L109-L113

    function transferFrom(address owner_, address recipient_, uint256 amount_) external override returns (bool success_) {
        _approve(owner_, msg.sender, allowance[owner_][msg.sender] - amount_);
        _transfer(owner_, recipient_, amount_);
        return true;
    }

See:

Recommendation

Change to:

    function transferFrom(address owner_, address recipient_, uint256 amount_) external override returns (bool success_) {
        uint256 currentAllowance = allowance[owner_][msg.sender];
        if (currentAllowance != type(uint256).max) {
            _approve(owner_, msg.sender, currentAllowance - amount_);
        }

        _transfer(owner_, recipient_, amount_);
        return true;
    }

[S] Use immutable variables can save gas

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/ERC20.sol#L25-L26

    string public override name;
    string public override symbol;

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/ERC20.sol#L50-L54

    constructor(string memory name_, string memory symbol_, uint8 decimals_) {
        name     = name_;
        symbol   = symbol_;
        decimals = decimals_;
    }

In ERC20.sol, name and symbol will never change, use immutable variable instead of storage variable can save gas.

[M] Validation can be done earlier to save gas

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/ERC20.sol#L75-L102

    function permit(address owner_, address spender_, uint256 amount_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_) external override {
        require(deadline_ >= block.timestamp, "ERC20:P:EXPIRED");

        // Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines
        // the valid range for s in (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}.
        require(
            uint256(s_) <= uint256(0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) &&
            (v_ == 27 || v_ == 28),
            "ERC20:P:MALLEABLE"
        );

        // Nonce realistically cannot overflow.
        unchecked {
            bytes32 digest = keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    DOMAIN_SEPARATOR(),
                    keccak256(abi.encode(PERMIT_TYPEHASH, owner_, spender_, amount_, nonces[owner_]++, deadline_))
                )
            );

            address recoveredAddress = ecrecover(digest, v_, r_, s_);

            require(recoveredAddress == owner_ && owner_ != address(0), "ERC20:P:INVALID_SIGNATURE");
        }

        _approve(owner_, spender_, amount_);
    }

Check if owner_ != address(0) earlier can avoid unnecessary computing when this check failed.

Recommendation

Change to:

    function permit(address owner_, address spender_, uint256 amount_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_) external override {
        require(deadline_ >= block.timestamp, "ERC20:P:EXPIRED");
        require(owner_ != address(0), "...");

        // Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines
        // the valid range for s in (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}.
        require(
            uint256(s_) <= uint256(0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) &&
            (v_ == 27 || v_ == 28),
            "ERC20:P:MALLEABLE"
        );

        // Nonce realistically cannot overflow.
        unchecked {
            bytes32 digest = keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    DOMAIN_SEPARATOR(),
                    keccak256(abi.encode(PERMIT_TYPEHASH, owner_, spender_, amount_, nonces[owner_]++, deadline_))
                )
            );

            address recoveredAddress = ecrecover(digest, v_, r_, s_);

            require(recoveredAddress == owner_, "ERC20:P:INVALID_SIGNATURE");
        }

        _approve(owner_, spender_, amount_);
    }
@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Mar 21, 2022
code423n4 added a commit that referenced this issue Mar 21, 2022
@lucas-manuel
Copy link
Collaborator

[S] ERC20.sol#transferFrom() Do not reduce approval on transferFrom if current allowance is type(uint256).max

The Wrapped Ether (WETH) ERC-20 contract has a gas optimization that does not update the allowance if it is the max uint.

The latest version of OpenZeppelin's ERC20 token contract also adopted this optimization.

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/ERC20.sol#L109-L113

    function transferFrom(address owner_, address recipient_, uint256 amount_) external override returns (bool success_) {
        _approve(owner_, msg.sender, allowance[owner_][msg.sender] - amount_);
        _transfer(owner_, recipient_, amount_);
        return true;
    }

Less clean, not added

@lucas-manuel
Copy link
Collaborator

[S] Use immutable variables can save gas

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/ERC20.sol#L25-L26

    string public override name;
    string public override symbol;

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/ERC20.sol#L50-L54

    constructor(string memory name_, string memory symbol_, uint8 decimals_) {
        name     = name_;
        symbol   = symbol_;
        decimals = decimals_;
    }

In ERC20.sol, name and symbol will never change, use immutable variable instead of storage variable can save gas.

Valid, will add

@lucas-manuel
Copy link
Collaborator

[M] Validation can be done earlier to save gas

https://github.com/maple-labs/erc20/blob/10ccf4aa0b2d6914e3c2d32e454e4d106a99a4fd/contracts/ERC20.sol#L75-L102

    function permit(address owner_, address spender_, uint256 amount_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_) external override {
        require(deadline_ >= block.timestamp, "ERC20:P:EXPIRED");

        // Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines
        // the valid range for s in (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}.
        require(
            uint256(s_) <= uint256(0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) &&
            (v_ == 27 || v_ == 28),
            "ERC20:P:MALLEABLE"
        );

        // Nonce realistically cannot overflow.
        unchecked {
            bytes32 digest = keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    DOMAIN_SEPARATOR(),
                    keccak256(abi.encode(PERMIT_TYPEHASH, owner_, spender_, amount_, nonces[owner_]++, deadline_))
                )
            );

            address recoveredAddress = ecrecover(digest, v_, r_, s_);

            require(recoveredAddress == owner_ && owner_ != address(0), "ERC20:P:INVALID_SIGNATURE");
        }

        _approve(owner_, spender_, amount_);
    }

Check if owner_ != address(0) earlier can avoid unnecessary computing when this check failed.

Less clean, won't add

@lucas-manuel
Copy link
Collaborator

Acknowledge 1 and 3
2 is valid

@lucas-manuel lucas-manuel added sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working G (Gas Optimization) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

2 participants