Skip to content

Gas optimization #6

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.

Sign up for GitHub

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

Open
juniormp wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Gas optimization #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
9dfb3ef
set up hardhat gas reporter and get gas cost benchmark
Mar 30, 2023
4fe8c0c
grt liquidity wallet gas optimization
Mar 31, 2023
0656d67
gas report liquidity pool before optimization
Mar 31, 2023
40a079c
gas optimization grt pool
Mar 31, 2023
41da6b6
remove file
Mar 31, 2023
47c121b
gas optimization report grt offer before changes
Apr 3, 2023
fadaf1a
gas optimization grt offer
Apr 3, 2023
9aeda15
gas optimization report after grt offer changes
Apr 3, 2023
e0f3423
gas optimization liquidity wallet
Apr 4, 2023
7e09d66
gas reporter config
Apr 4, 2023
03b74b3
use import statement instead of require
Apr 5, 2023
2885f4b
add modifier to check msg sender with offer user
Apr 17, 2023
a9f95d0
add validation for zero address
Apr 17, 2023
9376d32
move errors to top
Apr 17, 2023
6b9a6ed
refactor to use require statement
Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions abis/GrtLiquidityWallet.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,19 @@
[
{
"inputs": [],
"name": "FailedToSendNativeTokens",
"type": "error"
},
{
"inputs": [],
"name": "InsufficientBalance",
"type": "error"
},
{
"inputs": [],
"name": "NotAllowedToPayTheOffer",
"type": "error"
},
{
"anonymous": false,
"inputs": [
Expand Down
30 changes: 30 additions & 0 deletions abis/GrtPool.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,34 @@
[
{
"inputs": [],
"name": "FailedToSendNativeTokens",
"type": "error"
},
{
"inputs": [],
"name": "NotAllowedToModifyOffer",
"type": "error"
},
{
"inputs": [],
"name": "OfferInactive",
"type": "error"
},
{
"inputs": [],
"name": "TransferedAmountMustBePositive",
"type": "error"
},
{
"inputs": [],
"name": "ZeroAddressIsNotAllowed",
"type": "error"
},
{
"inputs": [],
"name": "ZeroAddressNotAllowed",
"type": "error"
},
{
"anonymous": false,
"inputs": [
Expand Down
56 changes: 27 additions & 29 deletions contracts/v-testnet-launch/GrtOffer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,56 +25,53 @@ contract GrtOffer is GrtOfferUtils {
);
event LogSetStatusOffer(bytes32 indexed _idOffer, bool indexed _isActive);

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

@tcoratger tcoratger Apr 19, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

require(
msg.sender == _offers[offerId].user,
msg.sender == offer.user,
"Grindery offer: you are not allowed to modify this offer."
);
_offers[offerId].chainId = chainId;
_;
}

function setChainIdOffer(bytes32 offerId, uint256 chainId) external isOwner(_offers[offerId]) {
Offer storage offer = _offers[offerId];
offer.chainId = chainId;
emit LogSetChainIdOffer(offerId, chainId);
}

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

@tcoratger tcoratger Apr 19, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

msg.sender == _offers[offerId].user,
"Grindery offer: you are not allowed to modify this offer."
);
_offers[offerId].token = token;
Offer storage offer = _offers[offerId];
offer.token = token;
emit LogSetTokenOffer(offerId, token);
}

function setMinPriceLimit(
bytes32 offerId,
bytes calldata minPriceLimit
) external {
require(
msg.sender == _offers[offerId].user,
"Grindery offer: you are not allowed to modify this offer."
);
) external isOwner(_offers[offerId]) {
Offer storage offer = _offers[offerId];
bytes32 priceLimit = keccak256(abi.encodePacked(minPriceLimit));
_offers[offerId].minPriceLimit = priceLimit;
offer.minPriceLimit = priceLimit;
emit LogSetMinPriceLimit(offerId, priceLimit);
}

function setMaxPriceLimit(
bytes32 offerId,
bytes calldata maxPriceLimit
) external {
require(
msg.sender == _offers[offerId].user,
"Grindery offer: you are not allowed to modify this offer."
);
) external isOwner(_offers[offerId]) {
Offer storage offer = _offers[offerId];
bytes32 priceLimit = keccak256(abi.encodePacked(maxPriceLimit));
_offers[offerId].maxPriceLimit = priceLimit;
offer.maxPriceLimit = priceLimit;
emit LogSetMaxPriceLimit(offerId, priceLimit);
}

function setIsActive(bytes32 offerId, bool isActive) external {
require(
msg.sender == _offers[offerId].user,
"Grindery offer: you are not allowed to modify this offer."
);
_offers[offerId].isActive = isActive;
function setIsActive(bytes32 offerId, bool isActive) external isOwner(_offers[offerId]) {
Offer storage offer = _offers[offerId];
offer.isActive = isActive;
emit LogSetStatusOffer(offerId, isActive);
}

Expand All @@ -91,14 +88,15 @@ contract GrtOffer is GrtOfferUtils {
bytes32 offerId = keccak256(
abi.encodePacked(msg.sender, _noncesOffer[msg.sender])
);
_offers[offerId].user = msg.sender;
_offers[offerId].isActive = true;
_offers[offerId].chainId = chainId;
_offers[offerId].token = token;
_offers[offerId].minPriceLimit = keccak256(
Offer storage offer = _offers[offerId];
offer.user = msg.sender;
offer.isActive = true;
offer.chainId = chainId;
offer.token = token;
offer.minPriceLimit = keccak256(
abi.encodePacked(minPriceLimit)
);
_offers[offerId].maxPriceLimit = keccak256(
offer.maxPriceLimit = keccak256(
abi.encodePacked(maxPriceLimit)
);
emit LogNewOffer(offerId, token, chainId);
Expand Down
13 changes: 2 additions & 11 deletions contracts/v-testnet-launch/GrtPool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ contract GrtPool is OwnableUpgradeable, GrtOffer, UUPSUpgradeable {

function _authorizeUpgrade(address) internal override onlyOwner onlyProxy {}

receive() external payable {}

function depositETHAndAcceptOffer(
bytes32 offerId,
address destAddr
Expand All @@ -54,7 +52,6 @@ contract GrtPool is OwnableUpgradeable, GrtOffer, UUPSUpgradeable {
_offers[offerId].isActive,
"Grindery Pool: the offer is inactive."
);

(bool sent, ) = address(this).call{value: msg.value}("");

@juniormp juniormp Apr 17, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tcoratger tcoratger Apr 18, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

require(sent, "Grindery Pool: failed to send native tokens.");
bytes32 tradeId = keccak256(
Expand All @@ -63,7 +60,7 @@ contract GrtPool is OwnableUpgradeable, GrtOffer, UUPSUpgradeable {
Trade storage trade = _trades[tradeId];
trade.userAddr = msg.sender;
trade.destAddr = destAddr;
trade.deposit = setTokenInfo(address(0), msg.value, block.chainid);
trade.deposit = TokenInfo(address(0), msg.value, block.chainid);
trade.offerId = offerId;
_noncesDeposit[msg.sender]++;
emit LogTrade(tradeId, address(0), msg.value, offerId);
Expand Down Expand Up @@ -100,11 +97,5 @@ contract GrtPool is OwnableUpgradeable, GrtOffer, UUPSUpgradeable {
return _trades[tradeId].deposit.chainId;
}

function setTokenInfo(
address token,
uint256 amount,
uint256 chainId
) internal pure returns (TokenInfo memory) {
return TokenInfo(token, amount, chainId);
}
receive() external payable {}
}
10 changes: 9 additions & 1 deletion hardhat.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import "./tasks/v-testnet-launch/deploy-grtPool";
import "./tasks/v-testnet-launch/deploy-grtLiquidityWallet";
import "./tasks/v-testnet-launch/update-grtLiquidityWallet";
import "./tasks/v-testnet-launch/update-grtPool";
import "hardhat-gas-reporter";

let protocolVersion = "-testnet-launch";

Expand Down Expand Up @@ -161,7 +162,14 @@ const config: HardhatUserConfig = {
spacing: 2,
format: "json",
},

gasReporter: {
//outputFile: "gas-report.txt",
enabled: process.env.REPORT_GAS !== undefined,
currency: "USD",
noColors: true,
// coinmarketcap: process.env.COIN_MARKETCAP_API_KEY || "",
token: "ETH"
}
// deterministicDeployment: () => {
// return {
// factory: contractAddress,
Expand Down
22 changes: 6 additions & 16 deletions test/v-testnet-launch/GrtOffer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { expect } from "chai";
import { ethers, upgrades } from "hardhat";
import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
import { Contract } from "ethers";
import { Contract, constants } from "ethers";

const protocolVersion = "v-testnet-launch";

Expand Down Expand Up @@ -213,9 +213,7 @@ describe("Grindery Offer testings", function () {
it("Should fail if the sender is not the creator of the offer", async function () {
await expect(
grtOffer.connect(user2).setChainIdOffer(offerId, 34)
).to.be.revertedWith(
"Grindery offer: you are not allowed to modify this offer."
);
).to.be.revertedWith("Grindery offer: you are not allowed to modify this offer.")
});

it("Should modify the chainID", async function () {
Expand All @@ -236,9 +234,7 @@ describe("Grindery Offer testings", function () {
it("Should fail if the sender is not the creator of the offer", async function () {
await expect(
grtOffer.connect(user2).setTokenOffer(offerId, token1.address)
).to.be.revertedWith(
"Grindery offer: you are not allowed to modify this offer."
);
).to.be.revertedWith("Grindery offer: you are not allowed to modify this offer.")
});

it("Should modify the token address", async function () {
Expand Down Expand Up @@ -269,9 +265,7 @@ describe("Grindery Offer testings", function () {
["FIRA", "50"]
)
)
).to.be.revertedWith(
"Grindery offer: you are not allowed to modify this offer."
);
).to.be.revertedWith("Grindery offer: you are not allowed to modify this offer.")
});

it("Should modify the Min price limit options", async function () {
Expand Down Expand Up @@ -341,9 +335,7 @@ describe("Grindery Offer testings", function () {
["FIRA", "2000"]
)
)
).to.be.revertedWith(
"Grindery offer: you are not allowed to modify this offer."
);
).to.be.revertedWith("Grindery offer: you are not allowed to modify this offer.")
});

it("Should modify the Max price limit options", async function () {
Expand Down Expand Up @@ -405,9 +397,7 @@ describe("Grindery Offer testings", function () {
it("Should fail if the sender is not the creator of the offer", async function () {
await expect(
grtOffer.connect(user2).setIsActive(offerId, false)
).to.be.revertedWith(
"Grindery offer: you are not allowed to modify this offer."
);
).to.be.revertedWith("Grindery offer: you are not allowed to modify this offer.")
});

it("Should modify the status", async function () {
Expand Down
4 changes: 1 addition & 3 deletions test/v-testnet-launch/GrtPool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,7 @@ describe("Grindery Offer testings", function () {
.depositETHAndAcceptOffer(offerId, user3.address, {
value: 0,
})
).to.be.revertedWith(
"Grindery Pool: transfered amount must be positive."
);
).to.be.revertedWith("Grindery Pool: transfered amount must be positive.");
});

it("Should fail if the offer is inactive", async function () {
Expand Down