Golom contest - ElKu's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L375-L403

Vulnerability details

Impact

In GolomTrader contract, the seller gets paid less than he is entitled to, if both the following points match:

1. The user uses ordertype 1 (fillBid function) OR ordertype 2 (fillCriteriaBid function) to execute the trade.
2. The user trades more than 1 ERC1155 token. The more tokens he trades (in a single transaction), the more eth he will loose due to the issue in the code.

As we will be proving, the seller losses amount-1 times the actual protocol fee charged by the contract, where amount is the number of ERC1155 tokens being traded in a single transaction.

This unsent fee is stuck in the GolomTrader contract, as there is no way to directly take away any eth stuck in the contract.

Proof of Concept

The fillBid function and fillCriteriaBid function in GolomTrader.sol calls an internal function called _settleBalances. This function takes the payment from the order.signer and sends it to various recipients such as protocol fee in eth to RewardDistributor, eth as exchange fees, extra payments, royalties etc.

Looking at the function, we calculate the protocol fee as follows:

//Note that the fee for all the tokens are calculated here by multiplying by "amount".
uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;

Then in the next few lines, we send eth to various addresses, which are correctly implemented. The mistake lies in the section where the seller(msg.sender) is sent his fees.

        if (o.refererrAmt > 0 && referrer != address(0)) {
            payEther(o.refererrAmt * amount, referrer);
            payEther(
here --->        (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
                    amount -
                    p.paymentAmt,
                msg.sender
            );
        } else {
            payEther(
here --->        (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
                msg.sender
            );
        }

See the two marked lines. The protocolfee previously calculated is subtracted from o.totalAmt (which was what we took from the order.signer in line 382.

When the protocolfee was calculated, we had already multiplied it by amount. But then this value is subtracted from o.totalAmt and then gets multiplied by amount again. Which reduces the eth sent to the seller, which is msg.sender in this case.

Test results printed out in Hardhat:

    #fillBid
Actual protocol fee:  0.0151
Expected Profit for maker(ETH):  0.2549
Actual ETH transferred to maker:  0.2398
Funds lost to maker and stuck forever in Trader contract(ETH):  0.0151
Funds lost divided by Actual protocol fee:  1
      ✓ fillBid tested with ERC1155 token- 2 tokens
Actual protocol fee:  0.03775
Expected Profit for maker(ETH):  1.01225
Actual ETH transferred to maker:  0.86125
Funds lost to maker and stuck forever in Trader contract(ETH):  0.151
Funds lost divided by Actual protocol fee:  4
      ✓ fillBid tested with ERC1155 token- 5 tokens
Actual protocol fee:  0.0755
Expected Profit for maker(ETH):  2.2745
Actual ETH transferred to maker:  1.595
Funds lost to maker and stuck forever in Trader contract(ETH):  0.6795
Funds lost divided by Actual protocol fee:  9
      ✓ fillBid tested with ERC1155 token- 10 tokens

You can see that we did 3 tests, one with 2 tokens, another with 5 tokens and another with 10 tokens. And in each test, ( Funds Lost by the User / Actual Protocol Fee ) is 1, 4 and 9 respectively. which is number of tokens-1.

The GolomTrader contract has the weth contract address hardcoded in it. For testing it, I had to pass the weth contract address from Hardhat during run time. So I made a small change in the GolomTrader contract to accept weth address in its constructor.

    constructor(address _governance, address _weth) {
        WETH = ERC20(_weth);

The diff between the provided GolomTrader.specs.ts file and the new one I used is here: diff file.

Tools Used

Hardhat, VSCode.

Recommended Mitigation Steps

Correct the formulas in this section as give below:

        if (o.refererrAmt > 0 && referrer != address(0)) {
            payEther(o.refererrAmt * amount, referrer);
            payEther(
              (o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
                    amount - protocolfee -
                    p.paymentAmt,
                msg.sender
            );
        } else {
            payEther(
             (o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - protocolfee - p.paymentAmt,
                msg.sender
            );
        }

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L71-L109

Vulnerability details

Impact

In VoteEscrowDelegation contract, the _writeCheckpoint internal function has major logic errors which will make the delegate external function to always revert.

Proof of Concept

Suppose an user calls the delegate function for the first time for a particular toTokenId. Since its the first time, the nCheckpoints for that token will be zero. Which means the else part of the condition will be executed. Here, we call the internal function _writeCheckpoint with a value of 0 for nCheckpoints parameter. Pasted below is the delegate function code with some extra comments:

    function delegate(uint256 tokenId, uint256 toTokenId) external {  
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');

        delegates[tokenId] = toTokenId;
--->>>  uint256 nCheckpoints = numCheckpoints[toTokenId];   //this will be zero for the first time.

        if (nCheckpoints > 0) {
            Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
            checkpoint.delegatedTokenIds.push(tokenId);
            _writeCheckpoint(toTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
--->>>  } else {      // so control goes here.
            uint256[] memory array = new uint256[](1);
            array[0] = tokenId;
--->>>      _writeCheckpoint(toTokenId, nCheckpoints, array);  //internal function called with 0 value for nCheckpoints parameter
        }

        emit DelegateChanged(tokenId, toTokenId, msg.sender);
    }

Now looking at the _writeCheckpoint internal function:

    function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {
        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
        //see the line below. "nCheckpoints - 1" is used as an index. Which results in underflow panic error.
-->>>   Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

        if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
            oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
        } else {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
        }
    }

<ins>The test in hardhat returns the following error message:</ins>

     Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)
      at VoteEscrow._writeCheckpoint (contracts/vote-escrow/VoteEscrowDelegation.sol:102)
      at VoteEscrow.delegate (contracts/vote-escrow/VoteEscrowDelegation.sol:87)
      at async HardhatNode._mineBlockWithPendingTxs (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:1772:23)
      at async HardhatNode.mineBlock (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:466:16)
      at async EthModule._sendTransactionAndReturnHash (node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:1496:18)
      at async HardhatNetworkProvider.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:118:18)
      at async EthersProviderWrapper.send (node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)

<ins>Test script used to test the code in Hardhat:</ins>

import { ethers, waffle } from 'hardhat';
import { BigNumber, utils, Signer, constants } from 'ethers';
import chai from 'chai';
const { expect } = chai;

// import typings
import { VoteEscrow as VoteEscrowTypes } from '../typechain/VoteEscrow';
import { ERC20Mock as ERC20MockTypes } from '../typechain/ERC20Mock';

let testErc20: ERC20MockTypes;
let veDelegation: VoteEscrowTypes;

let accounts: Signer[];

describe('VoteEscrowDelegation.sol', function () {
    beforeEach(async function () {
        accounts = await ethers.getSigners();

        const Lib = await ethers.getContractFactory("TokenUriHelper");
        const lib = await Lib.deploy();
        await lib.deployed();
        const VoteEscrowArtifacts = ethers.getContractFactory('VoteEscrow',{
            libraries: {
                TokenUriHelper : lib.address,
            },
          });
        const ERC20MockArtifacts = ethers.getContractFactory('ERC20Mock');

        testErc20 = (await (await ERC20MockArtifacts).deploy()) as ERC20MockTypes;
		veDelegation = (await (await VoteEscrowArtifacts).deploy(testErc20.address)) as VoteEscrowTypes;

    });

    describe('#Delegate', () => {

        it('adding a delegate', async () => {
			const receiver = await accounts[2].getAddress();  //the one who owns golom tokens
            await testErc20.connect(accounts[0]).mint(receiver, 10000)  //receiver gets 10000 golem tokens

            //VoteEscrowDelegation contract is allowed to withdraw receiver's tokens on his behalf
            await testErc20.connect(accounts[2]).approve(veDelegation.address, 10000)  

            // the receiver address locks 100 golom tokens for a period of 1000000.
            // create_lock(uint256 _value, uint256 _lock_duration) external 
			let nftid = await veDelegation.connect(accounts[2]).create_lock(
				100,
				1000000,
			) 

            // then he calls the VoteEscrowDelegation contract's delegate function
			await veDelegation.connect(accounts[2]).delegate(
				1,
				nftid.value,
			)
        });
	});
});

Further Comments:

Along with this obvious error, there is one more logical error which was noticed in the _writeCheckpoint function. If the control goes into the if part of the statement, the function doesn't cause any state changes. As the oldCheckpoint variable it modifies is a memory variable. So any changes will be erased when the function is done executing.

    function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {
        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

--->>>  Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

        if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
--->>>      oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
        } else {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
        }
    }

Tools Used

Hardhat, VSCode.

Recommended Mitigation Steps

Correct the _writeCheckpoint function as shown below to solved the pointed out errors:

    function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {
        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
        if (nCheckpoints > 0) {
            Checkpoint storage oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
            if(oldCheckpoint.fromBlock == block.number) { 
                oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
            }
        } else {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
        }
    }

Low Risk Issues

Summary Of Findings:

Details on Findings:

1. <ins>Add a function to recover extra ether stuck in GolomTrader</ins>

Though the contract is doing everything it can, to have a zero balance in the GolomTrader contract, there can be unseen issues with the code which might result in eth getting stuck in the contract. An example would be the bug in the _settleBalances function where less eth is sent to the msg.sender than supposed to(I have submitted this as a separate finding marking it as a high risk issue) and the extra eth continues to stay in the contract without being able to be withdrawn.

In such cases it would be nice to have a function which can withdraw the funds and use it among the community or for a wholesome purpose.

2. <ins>Conditions inside require doesnt match function logic </ins>

There are two instances of this:

a) <ins>The following require statement in fillBid function in GolomTrader.sol doesn't match the function logic</ins>

require(
            o.totalAmt * amount >
                (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt
        ); 

The protocol fee of 0.5% is not checked within the require statement. But at the end of function we call _settleBalances function where the protocol fee is taken out from this totalAmt*amount.

This will cause the require statement to pass, only to revert on a much later stage, wasting gas of the trader.

<ins>Mitigation</ins> would be to alter the require conditions as below:

require(
           o.totalAmt * amount >
               (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) /  10000) * amount + p.paymentAmt
       ); 

b) <ins>A similar issue is found in the fillCriteriaBid function too</ins>

require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);

Here the extra payment p and the protocol fees are not included in the conditions mentioned inside the require statement.

<ins>Mitigation would be to change the statement as follows:</ins>

require(
           o.totalAmt * amount >
               (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) /  10000) * amount + p.paymentAmt
       ); 

In fact all the three order types could have the same require conditions.

3. <ins>Timelock mechanism could be improved to be more secure and transparent</ins>

The timelock mechanism could have the following improvements:

<ins>a) Add an expiry timestamp to the timelock proposal</ins> This removes the possibility of an old proposal suddenly getting executed. Circumstances might have changed from the time when the proposal was made.

An example mitigation would be the setDistributor function from the GolomTrader.sol:

    function setDistributor(address _distributor) external onlyOwner {
        if (address(distributor) == address(0)) {
            distributor = Distributor(_distributor);
        } else {
            pendingDistributor = _distributor;
            distributorEnableDate = block.timestamp + 1 days;
            distributorDisableDate = block.timestamp + 2 days;  //see this. new expiry parameter added
        }
    }

And the corresponding executeSetDistributor function is also changed as shown below:

    function executeSetDistributor() external onlyOwner {
        require(distributorEnableDate <= block.timestamp, 'LOCKED');
        require(distributorDisableDate >= block.timestamp, 'EXPIRED');  //new require statement. EXPIRED
        distributor = Distributor(pendingDistributor);
    }

In the above example the proposal needs to be activated within one day of being unlocked.

There are 3 instances of this:

1. setDistributor in GolomTrader
2. changeTrader in RewardDistributor
3. addVoteEscrow in RewardDistributor
4. setMinter in GolomToken

<ins>b) An event should be emitted when a proposal is made and is executed</ins> This leads to a much more transparent system.

<ins>c) Older proposal can be overwritten by a new one without any warning</ins> Not sure if it was the intention of the developers, but it is something to be kept in mind.

4. <ins>Compare the value of startTime with block.timestamp in RewardDistributor.sol</ins>

The constructor in RewardDistributor.sol initializes startTime state variable with a magic number. I am assuming that this was done expecting the contract will be launched on a set date. But unexpected things can happen and its safer to apply some conditions programmatically to make sure we dont have the wrong value in there. For example:

uint256 _starttime = 1659211200; 
require(_starttime >= block.timestamp && _starttime <= block.timestamp + 1 days, "starttime out of range");
starttime = _starttime;

Setting the wrong value of starttime will cause the reward cycles to be calculated wrongly in the addFee function. This will avoid those complications, if the developer decides to deploy the contract earlier or later than originally intended.

5. <ins>Anyone can claim Trader/Exchange rewards on their owner's behalf(without permission)</ins>

In RewardDistributor.sol contract, a trader or exchange can claim their GOLOM token rewards with help of traderClaim and exchangeClaim public functions respectively.

These functions can be called by anyone and the contract will calculate the rewards belonging to the addr input for the given epoch's and send them to the particular address. Though this cannot result in loss of funds, it can be an unpleasant experience to the user that anybody can initiate his/her fund transfer without permission.

<ins>Mitigation</ins> would be to either check if the msg.sender and addr are the same. Or create an approval mapping to make sure that only a particular address can withdraw the funds for a particular trader/exchange address.

6. <ins>Missing timelock for MIN_VOTING_POWER_REQUIRED in VoteEscrowDelegation</ins>

As an important protocol parameter I suggest that a time lock functionality is added to the changeMinVotingPower function. Point 3 above could be referred for implementing a better timelock system.

7. <ins>Events are not emitted for Important state changes</ins>

The following functions doesn't emit an event upon completion:

1. function setMinter in GolomToken
2. function executeSetMinter in GolomToken
3. function setDistributor in GolomTrader
4. function executeSetDistributor in GolomTrader
5. function changeTrader in RewardDistributor
6. function executeChangeTrader in RewardDistributor
7. function addVoteEscrow in RewardDistributor
8. function executeAddVoteEscrow in RewardDistributor
9. function removeDelegation in VoteEscrowDelegation
10. function changeMinVotingPower in VoteEscrowDelegation

8. <ins>Once an order is cancelled it cannot be placed again(GolomTrader.sol)</ins>

In GolomTrader.sol, an order can be cancelled through the following cancelOrder function.

    function cancelOrder(Order calldata o) public nonReentrant {
        require(o.signer == msg.sender);
        (, bytes32 hashStruct, ) = validateOrder(o);
        filled[hashStruct] = o.tokenAmt + 1;
        emit OrderCancelled(hashStruct);
    }

The order is cancelled by making the filled value by 1 more than the o.tokenAmt.

If the user decides to place the order with the same parameters again, he/she won't be able to do it. As the validateOrder function will return a status value of 2, instead of 3, thus reverting the transaction.

Though this can be resolved by the user by changing the order parameters by a bit, it can lead to an unpleasant user experience in certain cases.

<ins>Mitigation</ins> would be to allow the user to cancel the cancellation of an order.

Non-critical Issues

Summary Of Findings:

Details on Findings:

1. <ins>Events should be emitted at the end</ins>

There are two instances of this.

In fillBid and fillCriteriaBid functions, the event is emitted before calling the _settleBalances function. To save any extra gas consumption on a reversal, its recommended to emit events at the end of the function, unless there is a specific reason not to do so. a) fillBid function:

        emit OrderFilled(msg.sender, o.signer, 1, hashStruct, o.totalAmt * amount);
        _settleBalances(o, amount, referrer, p);

b) fillCriteriaBid

        emit OrderFilled(msg.sender, o.signer, 2, hashStruct, o.totalAmt * amount);
        _settleBalances(o, amount, referrer, p);

<ins>Mitigation</ins> would be to change the order of the above statements. That is to place the emit statement after calling the _settleBalances function.

2. <ins>constant's should be defined rather than using magic numbers</ins>

There are 6 instances of this spread out in 4 files.

1. GolomToken.sol

File: contracts/governance/GolomToken.sol

// mintAirdrop function
44:    _mint(_airdrop, 150_000_000 * 1e18);

// mintGenesisReward function
52:    _mint(_rewardDistributor, 62_500_000 * 1e18);

2. VoteEscrowDelegation.sol

File: contracts/vote-escrow/VoteEscrowDelegation.sol

// _writeCheckpoint function
99:    require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

// mintGenesisReward function
52:    _mint(_rewardDistributor, 62_500_000 * 1e18);

3. RewardDistributor.sol

File: contracts/rewards/RewardDistributor.sol

// addFee function
100:    if (rewardToken.totalSupply() > 1000000000 * 10**18) {

4. GolomTrader.sol

File: contracts/core/GolomTrader.sol

// the protocol fee of "50" is used at several places
381:    uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;

3. <ins>Spelling mistakes in Natspec comments<ins>

There are 11 instances of this spread out in 3 files:

1. RewardDistributor.sol

File: contracts/rewards/RewardDistributor.sol

95:    facilated
168:   there

2. GolomTrader.sol

File: contracts/core/GolomTrader.sol

201:    wanna, succesful 
278:    wanna, succesful 
333:    wanna, succesful 
370:    succesfully, succesful 

3. VoteEscrowDelegation.sol

File: contracts/vote-escrow/VoteEscrowDelegation.sol

227:    Exeute 

4. <ins>Missing Natspec comments</ins>

There are 11 instances of this spread out in 2 files:

1. RewardDistributor.sol

File: contracts/rewards/RewardDistributor.sol

70:     event NewEpoch(uint256 indexed epochNo, uint256 tokenMinted, uint256 rewardStaker, uint256 previousEpochFee);

141:    function traderClaim(address addr, uint256[] memory epochs) public {

155:    function exchangeClaim(address addr, uint256[] memory epochs) public {

//missing natspec comments on return values.
215:    function stakerRewards(uint256 tokenid) public view returns (
254:    function traderRewards(address addr) public view returns (
269:    function exchangeRewards(address addr) public view returns (

2. GolomTrader.sol

File: contracts/core/GolomTrader.sol

79:    event NonceIncremented(address indexed maker, uint256 newNonce);

81:    event OrderFilled(
        address indexed maker,
        address indexed taker,
        uint256 indexed orderType,
        bytes32 orderHash,
        uint256 price
    );

89:    event OrderCancelled(bytes32 indexed orderHash);

164:   function validateOrder(Order calldata o)  

334:   function fillCriteriaBid(

5. <ins>require() statements should have descriptive reason strings</ins>

There are 17 instances of this spread out in 3 files:

1. RewardDistributor.sol

File: contracts/rewards/RewardDistributor.sol

88 :    require(msg.sender == trader);

144:    require(epochs[index] < epoch);

158:    require(epochs[index] < epoch);

2. GolomTrader.sol

File: contracts/core/GolomTrader.sol

211:    require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');  //mgmtm???

220:    require(msg.sender == o.reservedAddress);

285:    require(
            o.totalAmt * amount >
                (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + 
        p.paymentAmt
        ); 

291:    require(msg.sender == o.reservedAddress);

293:    require(o.orderType == 1);

295:    require(status == 3);

296:    require(amountRemaining >= amount);

313:    require(o.signer == msg.sender);

342:    require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);

345:    require(msg.sender == o.reservedAddress);

347:    require(o.orderType == 2);

349:    require(status == 3);

350:    require(amountRemaining >= amount);

3. VoteEscrowDelegation.sol

File: contracts/vote-escrow/VoteEscrowDelegation.sol

245:    require(_isApprovedOrOwner(_sender, _tokenId)); 

Gas Optimizations

Summary Of Findings:

Detailed Report on Gas Optimization Findings:

1. <ins>Cache calculations, which are used more than once</ins>

An example would be the use of the below formula:

(o.totalAmt * 50) / 10000

which is used several times in the fillAsk function in GolomTrader.sol. We could cache it as shown below:

uint256 protocolFee = (o.totalAmt * 50) / 10000;
// and then use the above variable in all the subsequent equations.
// for example instead of payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));
payEther(protocolFee * amount, address(distributor));  //we can use this

There are 5 places where we could replace this:

Line 211:    require(
            o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + 
         (o.totalAmt * 50) / 10000,
            'amt not matching'
        );

Line 242:    payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));

Line 252:    payEther(
                (o.totalAmt -
                    (o.totalAmt * 50) /
                    10000 -
                    o.exchange.paymentAmt -
                    o.prePayment.paymentAmt -
                    o.refererrAmt) * amount,
                o.signer
            );
        } else {
Line 254:   payEther(
                (o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount,
                o.signer
            );
        }

Line 269:  distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);

<ins>Hardhat Gas report:</ins> | | Deployment cost of GolomTrader | fillAsk function | -- | -- | -- Before the change: | 2013842 | 241406| After the change : | 1992605 | 240923| Saved gas fees : | 21237 | 483|

Similarly in fillBid function, the following pairs of statements can be combined into one, as shown below:

            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);
     //becomes
            ERC721(o.collection).transferFrom(msg.sender, o.signer, o.tokenId);

            ERC1155 nftcontract = ERC1155(o.collection);
            nftcontract.safeTransferFrom(msg.sender, o.signer, o.tokenId, amount, '');
     //becomes
            ERC1155(o.collection).safeTransferFrom(msg.sender, o.signer, o.tokenId, amount, '');

The same statements were changed in the fillCriteriaBid function too generate the below report. <ins>Hardhat Gas report:</ins>*

*Due to non-availability of test code, gas savings for fillBid and fillCriteriaBid functions were not calculated.

<ins>RewardDistributor</ins>

In RewardDistributor contract, in the addFee function, the following state variables could be cached: epoch (accessed 14 times), rewardToken.totalSupply() (accessed 4 times) and rewardToken.balanceOf(address(ve)) (accessed 2 times).

Storage read costs 100 gas while memory read is only 3 gas, saving us 97 gas approximately per access. So with the following changes, we could save at least (13+3+1)*97 = <ins>1649</ins> gas.

index c475948..d152df4 100755
--- a/original_addFee.sol
+++ b/modified_addFee.sol
@@ -1,41 +1,44 @@
     function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
         //console.log(block.timestamp,epoch,fee);
-        if (rewardToken.totalSupply() > 1000000000 * 10**18) {
+        uint256 tokenSupply = rewardToken.totalSupply();  // changed in 4 places
+        if (tokenSupply > 1000000000 * 10**18) {
             // if supply is greater then a billion dont mint anything, dont add trades
             return;
         }
 
         // if 24 hours have passed since last epoch change
+        uint256 epochCached = epoch;  //changed in 1 places
         if (block.timestamp > startTime + (epoch) * secsInDay) {
             // this assumes atleast 1 trade is done daily??????
             // logic to decide how much token to emit
             // emission = daily * (1 - (balance of locker/ total supply))  full if 0 locked and 0 if all locked
             // uint256 tokenToEmit = dailyEmission * rewardToken.balanceOf()/
             // emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining
-            uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
-                rewardToken.totalSupply();
-            uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();
+            uint256 veBalance = rewardToken.balanceOf(address(ve)); // changed in 2 places
+            uint256 tokenToEmit = (dailyEmission * (tokenSupply - veBalance)) /
+                tokenSupply;
+            uint256 stakerReward = (tokenToEmit * veBalance) / tokenSupply;
             // deposit previous epoch fee to weth for distribution to stakers
 
-            uint256 previousEpochFee = epochTotalFee[epoch];
-            epoch = epoch + 1;
-            rewardStaker[epoch] = stakerReward;
-            rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
-            rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;
+            uint256 previousEpochFee = epochTotalFee[epochCached];
+            epoch = ++epochCached;
+            rewardStaker[epochCached] = stakerReward;
+            rewardTrader[epochCached] = ((tokenToEmit - stakerReward) * 67) / 100;
+            rewardExchange[epochCached] = ((tokenToEmit - stakerReward) * 33) / 100;
             rewardToken.mint(address(this), tokenToEmit);
-            epochBeginTime[epoch] = block.number;
+            epochBeginTime[epochCached] = block.number;
             if (previousEpochFee > 0) {
-                if (epoch == 1){
+                if (epochCached == 1){
                     epochTotalFee[0] =  address(this).balance; // staking and trading rewards start at epoch 1, for epoch 0 all contract ETH balance is converted to staker rewards rewards.
                     weth.deposit{value: address(this).balance}();  
                 }else{
                     weth.deposit{value: previousEpochFee}();
                 }
             }
-            emit NewEpoch(epoch, tokenToEmit, stakerReward, previousEpochFee);
+            emit NewEpoch(epochCached, tokenToEmit, stakerReward, previousEpochFee);
         }
-        feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee;
-        feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee;
-        epochTotalFee[epoch] = epochTotalFee[epoch] + fee;
+        feesTrader[addr[0]][epochCached] = feesTrader[addr[0]][epochCached] + fee;
+        feesExchange[addr[1]][epochCached] = feesExchange[addr[1]][epochCached] + fee;
+        epochTotalFee[epochCached] = epochTotalFee[epochCached] + fee;
         return;
     }

2. <ins>for loops can be gas optimized</ins>

See the example below:

 for (uint256 i = 0; i < myarray.length; i++) {
   //code here
  }

//can be optimized to
 uint256 arrayLength = myarray.length;
 for (uint256 i; i < arrayLength; ) {
   //code here
   unchecked{
     ++i;
   }
  }

We can optimize the for loop in three ways:

1. Add unchecked {} for increment of loop indices.
2. Removing redundant initialization of i.
3. Cache the array length.

An example implementation of this in remix shows that every iteration can save around 75 gas.

There are 11 instances of this:

File:  contracts/core/GolomTrader.sol

Line 415:  for (uint256 i = 0; i < proof.length; i++) 


File:  contracts/rewards/RewardDistributor.sol

Line 143:  for (uint256 index = 0; index < epochs.length; index++)
Line 157:  for (uint256 index = 0; index < epochs.length; index++)
Line 180:  for (uint256 tindex = 0; tindex < tokenids.length; tindex++)
Line 183:  for (uint256 index = 0; index < epochs.length; index++)
Line 226:  for (uint256 index = 0; index < epochs.length; index++)
Line 258:  for (uint256 index = 0; index < epoch; index++)
Line 273:  for (uint256 index = 0; index < epoch; index++)

File:  contracts/vote-escrow/VoteEscrowDelegation.sol

Line 171:  for (uint256 index = 0; index < delegated.length; index++) 
Line 189:  for (uint256 index = 0; index < delegatednft.length; index++) 
Line 199:  for (uint256 i; i < _array.length; i++)

Assuming an average of 5 iterations per loop:

Approximate gas saved = 11 instances * 5 iterations per loop * 75 gas in each iteration = <ins>4125</ins> gas.

3. <ins>Use custom errors rather than require() strings to save gas</ins>

See the example implementation to see, how both the deployment and execution gas costs are saved by using custom errors.

The reference code shared above shows that:

1. when a require statement string which is less than 32 characters is converted to custom error we can save around 11000 gas in deployment cost. And around 55 gas is saved when the transaction reverts with a custom error. If the transaction doesn't revert, there is no gas saving.
2. when a require statement string which is more than 32 characters is converted to custom error we can save around 8500 gas in deployment cost. And more than 70 gas(depends on how long the string is) is saved when the transaction reverts with a custom error. If the transaction doesn't revert, there is no gas saving.

I changed 3 require strings into custom errors in the GolomTrader contract, just to test it out. And it showed each change saved around 7000 to 7500 gas. Which could be used to determine the approximate deployment gas savings in our case. And each function with at least one require string will save us a maximum of 55 gas on reversal, when custom errors are used.

The instances below shows where we could apply these gas savings:

File:  contracts/core/GolomTrader.sol

// 10 require statements spread among 5 functions.
// Deployment cost savings: 10 instances * 7000 = 70,000 gas saved.
// Gas saved on reversals : 5 functions * 55 = 275 gas saved.
Line 177:  require(signaturesigner == o.signer, 'invalid signature');
Line 211:  require(
            o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + 
           (o.totalAmt * 50) / 10000,
            'amt not matching'
             );
Line 217:  require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');
Line 222:  require(o.orderType == 0, 'invalid orderType');
Line 226:  require(status == 3, 'order not valid');
Line 227:  require(amountRemaining >= amount, 'order already filled');
Line 235:  require(amount == 1, 'only 1 erc721 at 1 time');
Line 299:  require(amount == 1, 'only 1 erc721 at 1 time');
Line 359:  require(amount == 1, 'only 1 erc721 at 1 time');
Line 455:  require(distributorEnableDate <= block.timestamp, 'not allowed');


File:  contracts/rewards/RewardDistributor.sol

// 7 require statements spread among 4 functions.
// Deployment cost savings: 7 instances * 7000 = 49,000 gas saved.
// Gas saved on reversals : 4 functions * 55 = (at least) 220 gas saved.
Line 173:  require(address(ve) != address(0), ' VE not added yet');
Line 181:  require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
Line 184:  require(epochs[index] < epoch, 'cant claim for future epochs');
Line 185:  require(claimed[tokenids[tindex]][epochs[index]] == 0, 'cant claim if already claimed');
Line 220:  require(address(ve) != address(0), ' VE not added yet');
Line 292:  require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
Line 309:  require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');

File:  contracts/vote-escrow/VoteEscrowDelegation.sol

// 7 require statements spread among 3 functions.
// Deployment cost savings: 7 instances * 7000 = 49,000 gas saved.
// Gas saved on reversals : 3 instances * 55 = (at least) 165 gas saved.
Line 072:  require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
Line 073:  require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
Line 099:  require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
Line 130:  require(blockNumber < block.number, 'VEDelegation: not yet determined');
Line 186:  require(blockNumber < block.number, 'VEDelegation: not yet determined');
Line 211:  require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
Line 239:  require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');  

So total deployment gas saved = 70000 + 49000 + 49000 = 168000. Dynamic gas saved = 275 + 220 + 165 = 660.

Report of Total Gas Saved: