Gas optimization by juniormp · Pull Request #6 · grindery-io/Depay-Reality

juniormp · 2023-03-31T17:06:20Z

GrtPool

Before gas optimization.

After gas optimization.

Savings

Methods

setIsActive: 33873 - 33735 = 138 gas
setOffer: 165779 - 165628 = 151 gas
depositETHAndAcceptOffer: 196657 - 173566 = 23091 gas
approve: 59656 - 46234 = 13422 gas
mint: 59656 - 59656 = 0 gas

Deployment

GrtPool: 1979188 - 1734279 = 244909 gas
ERC20Example: 669119 - 669119 = 0 gas

GrtOffer

before gas optimization.

Savings

Methods

setChainIdOffer: 35765 - 35729 = 36 gas
setIsActive: 33873 - 33735 = 138 gas
setMaxPriceLimit: 37303 - 37245 = 58 gas
setMinPriceLimit: 37344 - 37286 = 58 gas
setOffer: 165780 - 165629 = 151 gas
setTokenOffer: 36136 - 36069 = 67 gas
approve: 39127 - 39127 = 0 gas
mint: 55998 - 55998 = 0 gas

Deployment

GrtPool: 1734279 - 1734279 = 0 gas
ERC20Example: 669119 - 669119 = 0 gas

juniormp · 2023-04-17T16:09:12Z

@@ -54,10 +57,10 @@ contract GrtLiquidityWallet is OwnableUpgradeable, UUPSUpgradeable {
        address to,


@tcoratger do you think it's valid to check if the address is zero for token and to params?

@juniormp Yes we can do that to be sure that there is no mistake on this side!

No payment with a wrong token (zero address) is valid.

No payment to a zero address. It is unlikely that Lisa gave a payment address 0 but it is always good to check it: in this case the procedure cancels.

juniormp · 2023-04-17T16:12:15Z

+            revert ZeroAddressIsNotAllowed();
+        if(!_offers[offerId].isActive)
+            revert OfferInactive();
        (bool sent, ) = address(this).call{value: msg.value}("");


Maybe we can move this call to the bottom of the function to protect against reentrancy ?
And also to follow Solidity security Guidelines with Checks-Effects-Interactions pattern.
https://docs.soliditylang.org/en/v0.6.11/security-considerations.html

@juniormp I understand your point here but I don't think it applies in this case.

You point to the fact that an attacker could call this function recursively and since the rest of the calculations would not yet be finished, he could then call the function ten times.

First, in any case, the funds are transferred to our smart contract and not to an external account, so this attack would be rather beneficial for us :)

Then, I think it is important to keep this call here because this allows us to first make the deposit on our contract and then the following calculations (in the event that the deposit fails everything is canceled afterwards). Whereas if we position this call at the end, it is possible that all the calculations are done then that the deposit fails at the end and in this case it can cause a bug on our side.

@SAPikachu Maybe you have a better opinion here?

tcoratger · 2023-04-18T10:55:05Z

@juniormp Can you have a look at all the revert functions. Here are the explanations: we want to make a platform easy to use for non-developers. This must therefore include the most explicit and readable error messages possible for uninitiated users.

In this context, your revert functions don't contain explicit messages. It would be preferable to add something like this example:

Link: https://solidity-by-example.org/error/#:~:text=revert(%22Input%20must%20be%20greater%20than%2010%22)%3B

This is for our final user to read a proper sentence rather that a function name like TransferedAmountMustBePositive. What do you think?

juniormp · 2023-04-18T17:52:53Z

@juniormp Can you have a look at all the revert functions. Here are the explanations: we want to make a platform easy to use for non-developers. This must therefore include the most explicit and readable error messages possible for uninitiated users.

In this context, your revert functions don't contain explicit messages. It would be preferable to add something like this example:

Link: https://solidity-by-example.org/error/#:~:text=revert(%22Input%20must%20be%20greater%20than%2010%22)%3B
This is for our final user to read a proper sentence rather that a function name like TransferedAmountMustBePositive. What do you think?

@tcoratger makes sense.
So we can comeback with the require check ?

tcoratger · 2023-04-18T18:02:49Z

@tcoratger makes sense. So we can comeback with te require check ?

@juniormp Not obviously, there is also this option with an explicit message (to be seen in terms of transaction costs):

Link: https://solidity-by-example.org/error/#:~:text=revert(%22Input%20must%20be%20greater%20than%2010%22)%3B

On my side, I'm more used to seeing require.

juniormp · 2023-04-18T23:09:21Z

@juniormp Not obviously, there is also this option with an explicit message (to be seen in terms of transaction costs):

Link: https://solidity-by-example.org/error/#:~:text=revert(%22Input%20must%20be%20greater%20than%2010%22)%3B
On my side, I'm more used to seeing require.

I think the same. Therequire statement is the most common.

tcoratger · 2023-04-04T21:12:47Z

-            "Grindery wallet: insufficient balance."
-        );
+        if(address(this).balance < amount)
+            revert InsufficientBalance();


@juniormp Like for the other revert, it would be better to use a message in the form of a string that can be read directly by an average user who is not necessarily familiar with Solidity, like this example, WDT?

Link: https://solidity-by-example.org/error/#:~:text=revert(%22Input%20must%20be%20greater%20than%2010%22)%3B

@juniormp Can you have a look at this part when you have time so that we can merge before the audit (17 April)?

tcoratger · 2023-04-18T10:17:32Z

@@ -54,10 +57,10 @@ contract GrtLiquidityWallet is OwnableUpgradeable, UUPSUpgradeable {
        address to,


@juniormp Yes we can do that to be sure that there is no mistake on this side!

No payment with a wrong token (zero address) is valid.

No payment to a zero address. It is unlikely that Lisa gave a payment address 0 but it is always good to check it: in this case the procedure cancels.

tcoratger · 2023-04-18T10:50:56Z

+            revert ZeroAddressIsNotAllowed();
+        if(!_offers[offerId].isActive)
+            revert OfferInactive();
        (bool sent, ) = address(this).call{value: msg.value}("");


@juniormp I understand your point here but I don't think it applies in this case.

You point to the fact that an attacker could call this function recursively and since the rest of the calculations would not yet be finished, he could then call the function ten times.

First, in any case, the funds are transferred to our smart contract and not to an external account, so this attack would be rather beneficial for us :)

Then, I think it is important to keep this call here because this allows us to first make the deposit on our contract and then the following calculations (in the event that the deposit fails everything is canceled afterwards). Whereas if we position this call at the end, it is possible that all the calculations are done then that the deposit fails at the end and in this case it can cause a bug on our side.

@SAPikachu Maybe you have a better opinion here?

tcoratger · 2023-04-19T07:05:59Z

    event LogSetStatusOffer(bytes32 indexed _idOffer, bool indexed _isActive);

-    function setChainIdOffer(bytes32 offerId, uint256 chainId) external {
+    modifier isOwner(Offer memory offer) {


@juniormp A comment for detail but can you just change the name of this function to make it more explicit? Owner is already associated with a condition of the contract ownable and so this can be confusing when reading :)

tcoratger · 2023-04-19T07:08:37Z


-    function setTokenOffer(bytes32 offerId, address token) external {
+    function setTokenOffer(bytes32 offerId, address token) external isOwner(_offers[offerId]) {
        require(


@juniormp If we have the isOwner modifier, is there a reason to keep the require? I think that's a duplication, right?

Mauricio Junior added 10 commits March 30, 2023 20:26

set up hardhat gas reporter and get gas cost benchmark

9dfb3ef

grt liquidity wallet gas optimization

4fe8c0c

gas report liquidity pool before optimization

0656d67

gas optimization grt pool

40a079c

remove file

41da6b6

gas optimization report grt offer before changes

47c121b

gas optimization grt offer

fadaf1a

gas optimization report after grt offer changes

9aeda15

gas optimization liquidity wallet

e0f3423

gas reporter config

7e09d66

tcoratger reviewed Apr 4, 2023

View reviewed changes

Comment thread hardhat.config.ts Outdated

Mauricio Junior added 4 commits April 5, 2023 11:09

use import statement instead of require

03b74b3

add modifier to check msg sender with offer user

2885f4b

add validation for zero address

a9f95d0

move errors to top

9376d32

juniormp commented Apr 17, 2023

View reviewed changes

juniormp force-pushed the gas-optimization branch 2 times, most recently from 4d8bdc7 to 6b9a6ed Compare April 18, 2023 23:35

refactor to use require statement

6b9a6ed

tcoratger reviewed Apr 19, 2023

View reviewed changes

		@@ -54,10 +57,10 @@ contract GrtLiquidityWallet is OwnableUpgradeable, UUPSUpgradeable {
		address to,

Uh oh!

Conversation

juniormp commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GrtPool

Savings

GrtOffer

Savings

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcoratger commented Apr 18, 2023

Uh oh!

juniormp commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcoratger commented Apr 18, 2023

Uh oh!

juniormp commented Apr 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

juniormp commented Mar 31, 2023 •

edited

Loading

juniormp commented Apr 18, 2023 •

edited

Loading