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

Open
code423n4 opened this issue May 4, 2022 · 0 comments
Open

Gas Optimizations #71

code423n4 opened this issue May 4, 2022 · 0 comments
Labels
bug Something isn't working G (Gas Optimization)

Comments

@code423n4
Copy link
Contributor

Gas

TOC

  1. Reentrancy guard for mint function is redundant
  2. ForgottenRunesWarriorsGuild.sol mint improvement
  3. publicSummon duplicate require check
  4. IssueRefund send batch improvement

Reentrancy guard for mint function is redundant

ForgottenRunesWarriorsGuild.sol can remove nonReentrant modifier because Minter contract already have one and only minter can call mint function.

If worry about reentrancy attack, then move line numMinted +=1 to above _safeMint() function to prevent callback inside ERC721 contract.

ForgottenRunesWarriorsGuild.sol mint improvement

Replace these line with

    // save 100 gas from not calling cache number again.    
    uint256 tokenId = numMinted++; // This also prevent any reentrancy attack from minting same token twice
    _safeMint(recipient, tokenId);     

publicSummon duplicate require check

    require(numSold < maxForSale, 'Sold out');
    require(numSold + numWarriors <= maxForSale, 'Not enough remaining');

Both require check if sale number pass maxForSale. Only 2nd require is necessary.
First require only useful for information that user might never see.

IssueRefund send batch improvement

  • First, there is no need for refunding with WETH as it complicated thing, and it is not necessary. User should own up their mistake for using contract does not have default Ether fallback.
  • Second, use SafeTransferLib instead of current _safeTransferETH()

Improvement based on DisperseApp and SafeTransferLib. New batch refund cut around ~380 gas per address.

Replace issueRefund with:

    
    function issueRefunds(uint256 startIdx, uint256 endIdx)
        public
        onlyOwner
        nonReentrant
    {
        require(selfRefundsStarted(), 'Self refund period not started'); //Prevent accident refund during auction as price might change.
        uint256 end = endIdx + 1;
        for (uint256 i = startIdx; i < end; i++) {
            address minter = daMinters[i];
            uint256 owed = refundOwed(minter);
            if (owed > 0) {
                daAmountRefunded[minter] += owed;
                safeTransferETH(minter, owed);
            }
        }
    }

    function safeTransferETH(address to, uint256 amount) internal {
        bool success;
        assembly {
            // I agree that refund call should not forward all gas()
            // Contract DOS fallback can spend all gas received. 
            success:= call(30000, to, amount, 0, 0, 0, 0)
        }
        // save WETH as fallback for griefer in batch call is unnecessary. User can call selfRefund to get WETH as fallback.
        // Because if the receiver contract do not accept Ether fallback, what will happen if they do not support ERC20 withdrawal as well. The money will be stuck.
        if(!success) {
            IWETH(weth).deposit{value: amount}();            
            IERC20(weth).transfer(to, amount);        
        }
    }
@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels May 4, 2022
code423n4 added a commit that referenced this issue May 4, 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)
Projects
None yet
Development

No branches or pull requests

1 participant