code-423n4 / 2022-02-jpyc-findings

1 stars 0 forks source link

Gas Optimizations #40

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

> 0 is less efficient than != 0 for uint in require condition

Ref: https://twitter.com/GalloDaSballo/status/1485430908165443590

contracts/v1/FiatTokenV1.sol:136:        require(_amount > 0, "FiatToken: mint amount not greater than 0");
contracts/v1/FiatTokenV1.sol:368:        require(_amount > 0, "FiatToken: burn amount not greater than 0");
contracts/v2/FiatTokenV2.sol:143:        require(_amount > 0, "FiatToken: mint amount not greater than 0");
contracts/v2/FiatTokenV2.sol:378:        require(_amount > 0, "FiatToken: burn amount not greater than 0");

Use custom errors

Solidity ^0.8.4 allow the use of custom errors to optimize gas usage. https://blog.soliditylang.org/2021/04/21/custom-errors/

Unnecessary checks

Don't think this risk justisfy the additional gas cost https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v1/FiatTokenV1.sol#L245 https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v2/FiatTokenV2.sol#L253

        require(owner != address(0), "ERC20: approve from the zero address");

Use uint instead of bool

Instead of bool(true), we can use uint(1) instead. This save us around 27 gas each time whitelist is checked. https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v2/FiatTokenV2.sol#L62

        mapping(address => bool) internal whitelisted;

to

        mapping(address => uint) internal whitelisted;

and

        whitelisted[_account] = 1;
        whitelisted[_account] = 0;
        require(whitelisted[_account] != 0);

accordingly

Unchecked safe math

This is safe because individual balance cannot > totalSupply https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v1/FiatTokenV1.sol#L145 https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v2/FiatTokenV2.sol#L152

        balances[_to] = balances[_to] + _amount;

This is safe becuase of the prior value <= allowed[from][msg.sender] check https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v1/FiatTokenV1.sol#L276 https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v2/FiatTokenV2.sol#L286

        allowed[from][msg.sender] = allowed[from][msg.sender] - value;

This is safe because of the prior value <= balances[from] check https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v1/FiatTokenV1.sol#L316 https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v2/FiatTokenV2.sol#L326

        balances[from] = balances[from] - value;

Safe after reorder since individual balance cannot > totalSupply https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v1/FiatTokenV1.sol#L371 https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v2/FiatTokenV2.sol#L381

        balances[msg.sender] = balance - _amount;
        unchecked{
                totalSupply_ = totalSupply_ - _amount;
        }
retocrooman commented 2 years ago

Unnecessary checks

We will fix it. thank you. Do you think _transfer should also be fixed? https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L309