c4: gas optimization fixes #742

tmigone · 2022-10-24T17:13:45Z

See https://www.notion.so/edgeandnode/Code4rena-gas-report-notes-3cae3d2fc0304724aec0311b791fa1a7 for details.

Signed-off-by: Tomás Migone <[email protected]>

… extra gas Signed-off-by: Tomás Migone <[email protected]>

…fic string literal once Signed-off-by: Tomás Migone <[email protected]>

…nt in a require() Signed-off-by: Tomás Migone <[email protected]>

Signed-off-by: Tomás Migone <[email protected]>

…guments should be at the top of the function Signed-off-by: Tomás Migone <[email protected]>

…ak256(), should use immutable rather than constant Signed-off-by: Tomás Migone <[email protected]>

pcarranzav

Looking good, just a couple details

contracts/governance/Governed.sol

contracts/governance/Managed.sol

tmigone · 2022-10-24T18:49:50Z

@pcarranzav wait im not done yet 😵‍💫

codecov · 2022-10-24T18:55:40Z

Codecov Report

Base: 91.58% // Head: 91.58% // No change to project coverage 👍

Coverage data is based on head (e3a668e) compared to base (85312a0).
Patch coverage: 96.15% of modified lines in pull request are covered.

Additional details and impacted files

@@                      Coverage Diff                      @@
##           pcv/c4-699-m149-initializers     #742   +/-   ##
=============================================================
  Coverage                         91.58%   91.58%           
=============================================================
  Files                                42       42           
  Lines                              1997     1997           
  Branches                            357      359    +2     
=============================================================
  Hits                               1829     1829           
  Misses                              168      168

Flag	Coverage Δ
unittests	`91.58% <96.15%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
contracts/gateway/GraphTokenGateway.sol	`100.00% <ø> (ø)`
contracts/governance/Managed.sol	`94.54% <85.71%> (ø)`
contracts/gateway/BridgeEscrow.sol	`100.00% <100.00%> (ø)`
contracts/gateway/L1GraphTokenGateway.sol	`100.00% <100.00%> (ø)`
contracts/governance/Governed.sol	`100.00% <100.00%> (ø)`
contracts/l2/gateway/L2GraphTokenGateway.sol	`100.00% <100.00%> (ø)`
contracts/l2/token/GraphTokenUpgradeable.sol	`100.00% <100.00%> (ø)`
contracts/l2/token/L2GraphToken.sol	`100.00% <100.00%> (ø)`
contracts/upgrades/GraphProxy.sol	`100.00% <100.00%> (ø)`
contracts/upgrades/GraphUpgradeable.sol	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…red to a modifier or function Signed-off-by: Tomás Migone <[email protected]>

Signed-off-by: Tomás Migone <[email protected]>

pcarranzav · 2022-10-24T18:59:43Z

Ah sorry I jumped the gun again @tmigone 😆

Signed-off-by: Tomás Migone <[email protected]>

tmigone · 2022-10-24T19:22:31Z

To be fair i didn't set it to draft :P

pcarranzav

LGTM 💯

Would be nice to get an extra pair of eyes on the assembly changes in GraphProxy though 😨

tmigone · 2022-10-24T20:09:53Z

yes, that's so scary that I wondered wether or not to include them

cc @abarmat

abarmat · 2022-10-24T20:12:45Z

contracts/gateway/BridgeEscrow.sol

@@ -36,7 +36,6 @@ contract BridgeEscrow is Initializable, GraphUpgradeable, Managed {
     * @param _spender Address of the spender that will be revoked
     */
    function revokeAll(address _spender) external onlyGovernor {


@tmigone do we have a test for calling revokeAll?

yes we have one here:

contracts/test/gateway/bridgeEscrow.test.ts

Lines 67 to 75 in 0f553c9

it("revokes a spender's permission to transfer GRT held by the contract", async function () {

await bridgeEscrow.connect(governor.signer).approveAll(spender.address)

await bridgeEscrow.connect(governor.signer).revokeAll(spender.address)

// We shouldn't be able to transfer _anything_

const tx = grt

.connect(spender.signer)

.transferFrom(bridgeEscrow.address, tokenReceiver.address, BigNumber.from('1'))

await expect(tx).revertedWith('ERC20: transfer amount exceeds allowance')

})

abarmat · 2022-10-24T21:04:50Z

contracts/governance/Managed.sol

@@ -28,6 +28,14 @@ contract Managed {
    mapping(bytes32 => address) private addressCache;
    uint256[10] private __gap;

+    // Immutables


Is it better to be immutable than const? as there are no parametrization happening

I think in this case it's the same as we are using bytes32 to store the value (https://docs.soliditylang.org/en/v0.8.11/contracts.html#constant-and-immutable-state-variables).

I went with immutable because the reports suggested it, but should be the same as far as I understand.

…y approved amount Signed-off-by: Tomás Migone <[email protected]>

Signed-off-by: Tomás Migone <[email protected]>

abarmat · 2022-10-27T16:11:00Z

contracts/l2/gateway/L2GraphTokenGateway.sol

@@ -238,8 +238,7 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran
        if (_data.length > 0) {
            bytes memory callhookData;
            {
-                bytes memory gatewayData;
-                (gatewayData, callhookData) = abi.decode(_data, (bytes, bytes));
+                (, callhookData) = abi.decode(_data, (bytes, bytes));


@pcarranzav do you remember the content of gatewayData? where was this coming from?

Yeah that data is just sent by the Arbitrum router when creating the default bridge + L2 tokens, so we don't need it but we keep the format for compatibility

So in more detail:

The data is decoded here: https://github.com/OffchainLabs/arbitrum/blob/master/packages/arb-bridge-peripherals/contracts/tokenbridge/arbitrum/gateway/L2ArbitrumGateway.sol#L235-L255

And then used here: https://github.com/OffchainLabs/arbitrum/blob/master/packages/arb-bridge-peripherals/contracts/tokenbridge/arbitrum/gateway/L2ERC20Gateway.sol#L79-L102

Which then initializes the L2 token here: https://github.com/OffchainLabs/arbitrum/blob/288488a64d0510d7eb7f9f83b346fa00feb1bb02/packages/arb-bridge-peripherals/contracts/tokenbridge/arbitrum/StandardArbERC20.sol#L44-L80

trust1995 · 2022-11-22T11:06:10Z

contracts/gateway/L1GraphTokenGateway.sol

        require(_amount != 0, "INVALID_ZERO_AMOUNT");
+        require(_l1Token == address(token), "TOKEN_NOT_GRT");


Isn't it better to delay this check after the next line as well?

Good catch, noted to be added in a future PR

tmigone added 4 commits October 24, 2022 12:40

fix(c4-117-g03): state variables should be cached in stack variables

2c4ac9d

Signed-off-by: Tomás Migone <[email protected]>

fix(c4-117-g05): require()/revert() strings longer than 32 bytes cost…

76865a4

… extra gas Signed-off-by: Tomás Migone <[email protected]>

fix(c4-117-g06): keccak256() should only need to be called on a speci…

18afb3a

…fic string literal once Signed-off-by: Tomás Migone <[email protected]>

fix(c4-117-g11): using > 0 costs more gas than != 0 when used on a ui…

ee5d3cb

…nt in a require() Signed-off-by: Tomás Migone <[email protected]>

tmigone changed the base branch from dev to pcv/c4-699-m149-initializers October 24, 2022 17:14

tmigone changed the title ~~Tmigone/c4 gas~~ c4: gas optimization fixes Oct 24, 2022

tmigone added 3 commits October 24, 2022 15:10

fix(c4-117-g12): splitting require() statements that use && saves gas

71314dd

Signed-off-by: Tomás Migone <[email protected]>

fix(c4-117-g15): require() or revert() statements that check input ar…

d5aece4

…guments should be at the top of the function Signed-off-by: Tomás Migone <[email protected]>

fix(c4-6-g07): expressions for constant values such as a call to kecc…

2ad9f4e

…ak256(), should use immutable rather than constant Signed-off-by: Tomás Migone <[email protected]>

pcarranzav reviewed Oct 24, 2022

View reviewed changes

contracts/governance/Governed.sol Outdated Show resolved Hide resolved

contracts/governance/Managed.sol Outdated Show resolved Hide resolved

tmigone marked this pull request as draft October 24, 2022 18:49

tmigone added 2 commits October 24, 2022 15:56

fix(c4-6-g08): duplicated require()/revert() checks should be refacto…

e0440a8

…red to a modifier or function Signed-off-by: Tomás Migone <[email protected]>

fix(c4-6-13): use calldata instead of memory for function parameters

3805ca9

Signed-off-by: Tomás Migone <[email protected]>

fix(c4-86-g07): emitting or returning storage values

54157a7

Signed-off-by: Tomás Migone <[email protected]>

tmigone marked this pull request as ready for review October 24, 2022 19:41

tmigone requested review from pcarranzav and abarmat October 24, 2022 19:41

pcarranzav approved these changes Oct 24, 2022

View reviewed changes

abarmat reviewed Oct 24, 2022

View reviewed changes

abarmat added the audit-fix label Oct 25, 2022

tmigone added 4 commits October 27, 2022 12:07

fix(c4-172-g02): set allowance to 0 instead of decreasing allowance b…

7d29d6d

…y approved amount Signed-off-by: Tomás Migone <[email protected]>

fix(c4-287-g01): remove unused local variable

534d224

Signed-off-by: Tomás Migone <[email protected]>

fix(c4-272): remove unnecessary use of safe math

0b7ad55

Signed-off-by: Tomás Migone <[email protected]>

fix(c4-272): remove unnecessary usage of safemath

d091781

Signed-off-by: Tomás Migone <[email protected]>

tmigone added 3 commits October 27, 2022 12:07

fix: further optimize governed contract

1f85e12

Signed-off-by: Tomás Migone <[email protected]>

fix: add visibility to managed immutable vars

27902cc

Signed-off-by: Tomás Migone <[email protected]>

fix: update e2e tests to new error messages

e3a668e

Signed-off-by: Tomás Migone <[email protected]>

tmigone force-pushed the tmigone/c4-gas branch from 0f553c9 to e3a668e Compare October 27, 2022 15:07

abarmat reviewed Oct 27, 2022

View reviewed changes

abarmat approved these changes Oct 28, 2022

View reviewed changes

trust1995 reviewed Nov 22, 2022

View reviewed changes

Base automatically changed from pcv/c4-699-m149-initializers to pcv/l2-bridge November 23, 2022 14:24

pcarranzav merged commit e3a668e into pcv/l2-bridge Nov 23, 2022

pcarranzav deleted the tmigone/c4-gas branch November 23, 2022 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c4: gas optimization fixes #742

c4: gas optimization fixes #742

tmigone commented Oct 24, 2022 •

edited

Loading

pcarranzav left a comment

tmigone commented Oct 24, 2022

codecov bot commented Oct 24, 2022 •

edited

Loading

pcarranzav commented Oct 24, 2022

tmigone commented Oct 24, 2022

pcarranzav left a comment

tmigone commented Oct 24, 2022

abarmat Oct 24, 2022

tmigone Oct 24, 2022

abarmat Oct 24, 2022

tmigone Oct 24, 2022 •

edited

Loading

abarmat Oct 27, 2022

pcarranzav Oct 27, 2022

pcarranzav Oct 27, 2022

trust1995 Nov 22, 2022

pcarranzav Nov 23, 2022

	it("revokes a spender's permission to transfer GRT held by the contract", async function () {
	await bridgeEscrow.connect(governor.signer).approveAll(spender.address)
	await bridgeEscrow.connect(governor.signer).revokeAll(spender.address)
	// We shouldn't be able to transfer _anything_
	const tx = grt
	.connect(spender.signer)
	.transferFrom(bridgeEscrow.address, tokenReceiver.address, BigNumber.from('1'))
	await expect(tx).revertedWith('ERC20: transfer amount exceeds allowance')
	})

		require(_amount != 0, "INVALID_ZERO_AMOUNT");
		require(_l1Token == address(token), "TOKEN_NOT_GRT");

c4: gas optimization fixes #742

c4: gas optimization fixes #742

Conversation

tmigone commented Oct 24, 2022 • edited Loading

pcarranzav left a comment

Choose a reason for hiding this comment

tmigone commented Oct 24, 2022

codecov bot commented Oct 24, 2022 • edited Loading

Codecov Report

pcarranzav commented Oct 24, 2022

tmigone commented Oct 24, 2022

pcarranzav left a comment

Choose a reason for hiding this comment

tmigone commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmigone Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmigone commented Oct 24, 2022 •

edited

Loading

codecov bot commented Oct 24, 2022 •

edited

Loading

tmigone Oct 24, 2022 •

edited

Loading