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

Not using Proxy pattern #854

Open
codehawks-bot opened this issue Aug 6, 2023 · 1 comment
Open

Not using Proxy pattern #854

codehawks-bot opened this issue Aug 6, 2023 · 1 comment

Comments

@codehawks-bot
Copy link

Not using Proxy pattern

Severity

Gas Optimization / Informational

Relevant GitHub Links

Escrow escrow = new Escrow{salt: salt}(
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
if (address(escrow) != computedAddress) {
revert EscrowFactory__AddressesDiffer();
}
emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);
return escrow;

Summary

Instead of using the proxy pattern for new Escrow contracts, the Escrow contract is deployed every time. This leads to unwanted gas expenditure.

Vulnerability Details

Although all Escrow contracts have the same code, a new contract is deployed everytime instead of deploying a single implementation followed by proxies.

    function newEscrow(
        uint256 price,
        IERC20 tokenContract,
        address seller,
        address arbiter,
        uint256 arbiterFee,
        bytes32 salt
    ) external returns (IEscrow) {
        .......
       Escrow escrow = new Escrow{salt: salt}(
            price,
            tokenContract,
            msg.sender, 
            seller,
            arbiter,
            arbiterFee
        );
        if (address(escrow) != computedAddress) {
            revert EscrowFactory__AddressesDiffer();
        }
        emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);
        return escrow;
    }

Impact

Unwanted gas expenditure of about 400000 for every new escrow contract.

Tools Used

Foundry gas reported

Recommendations

Have a single escrow implementation contract and then deploy proxies.

@PatrickAlphaC
Copy link
Member

Would need to double check on this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants