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 #36

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

Gas Optimizations #36

code423n4 opened this issue Mar 21, 2022 · 5 comments
Labels
bug Something isn't working G (Gas Optimization) sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

#1 Using mutiple require instead of using operator && for saving gas

instead of using operator && on single require check. using mutiple require check can save more gas.

POC it can be seen from this report here :

code-423n4/2022-01-timeswap-findings#89

Tool Used

remix

Mitigation Step

occurence :

ERC20.sol #L101
Mappleloan.sol #L233

e.g:

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

change to

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

#2. Using nonces[owner_] can saving more gas

https://github.com/maple-labs/erc20/blob/main/contracts/ERC20.sol#L92

it can be simplified using nonces[owner_] instead of using nonces[owner_]++ and saving more gas for deploy but it would be decreasing the nonce.

##Tool Used

Manual Review / Remix

##Mitigation Step

Remove ++

#3. using > instead of >=

instead of using >= it can be use > so this implementation can saving more gas.

##Recommended Mitigation Step

change >= into >

##Occurance

xMPL.sol #58
ERC20.sol #L76
Mapleloan.sol #116

#4.unused _transfer & _approve in internal function and change into external for saving more gas

https://github.com/maple-labs/erc20/blob/main/contracts/ERC20.sol

in solmate ERC20 internal function was used for only mint and burning logic, but in ERC20.sol was used approve and transfer. but it can be the same solmate ERC20 implementation for saving more gas

##POC
https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC20.sol

Tool Used

Manual Review / Remix.

Recommended Mitigation Step

E.x :

Remove _transfer from internal function and use this instead

    function transfer(address to, uint256 amount) external override returns (bool) {
        balanceOf[msg.sender] -= amount;

        // Cannot overflow because the sum of all user
        // balances can't exceed the max uint256 value.
        unchecked {
            balanceOf[to] += amount;
        }

        emit Transfer(msg.sender, to, amount);

        return true;
    }

#5. State variables can be set as immutable

in the following file xMPL.sol, variable that could be set immutable to save gas

##Tool used

Manual Review

Mitigation Step

add Immutable

xMPL.sol scheduledMigrator  
xML.sol scheduledNewAsset
@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

#1 Using mutiple require instead of using operator && for saving gas

instead of using operator && on single require check. using mutiple require check can save more gas.

POC it can be seen from this report here :

code-423n4/2022-01-timeswap-findings#89

Tool Used

remix

Mitigation Step

occurence :

ERC20.sol #L101
Mappleloan.sol #L233

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

change to

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

Tested this out and it was the same when optimizations were used. Not valid.

@lucas-manuel
Copy link
Collaborator

#2. Using nonces[owner_] can saving more gas

https://github.com/maple-labs/erc20/blob/main/contracts/ERC20.sol#L92

it can be simplified using nonces[owner_] instead of using nonces[owner_]++ and saving more gas for deploy but it would be decreasing the nonce.

##Tool Used

Manual Review / Remix

##Mitigation Step

Remove ++

This would break functionality.

Invalid.

@lucas-manuel
Copy link
Collaborator

#3. using > instead of >=

instead of using >= it can be use > so this implementation can saving more gas.

##Recommended Mitigation Step

change >= into >

##Occurance

xMPL.sol #58
ERC20.sol #L76
Mapleloan.sol #116

This would break functionality.

Invalid.

@lucas-manuel
Copy link
Collaborator

#4.unused _transfer & _approve in internal function and change into external for saving more gas

https://github.com/maple-labs/erc20/blob/main/contracts/ERC20.sol

in solmate ERC20 internal function was used for only mint and burning logic, but in ERC20.sol was used approve and transfer. but it can be the same solmate ERC20 implementation for saving more gas

##POC https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC20.sol

Leaving as is, internal functions are cleaner, probably is valid though

@lucas-manuel
Copy link
Collaborator

#5. State variables can be set as immutable

in the following file xMPL.sol, variable that could be set immutable to save gas

##Tool used

Manual Review

Mitigation Step

add Immutable

xMPL.sol scheduledMigrator  
xML.sol scheduledNewAsset

These are variables. Invalid.

@lucas-manuel lucas-manuel added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label 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 disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

2 participants