Golom contest - rbserver's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L298-L305 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L308-L311 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L112-L114 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L172-L173 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L215-L220

Vulnerability details

Impact

When the RewardDistributor contract is deployed, ve and pendingVoteEscrow are both uninitialized. To set ve, the following addVoteEscrow and executeAddVoteEscrow functions need to be called. When calling addVoteEscrow, the address(ve) == address(0) condition will always be true so ve = VE(pendingVoteEscrow) will always be executed, and pendingVoteEscrow can never be assigned to a non-zero address. Because pendingVoteEscrow is always address(0), ve will always correspond to address(0) no matter how addVoteEscrow and executeAddVoteEscrow are called.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L298-L305

function addVoteEscrow(address _voteEscrow) external onlyOwner {
	if (address(ve) == address(0)) {
		ve = VE(pendingVoteEscrow);
	} else {
		voteEscrowEnableDate = block.timestamp + 1 days;
		pendingVoteEscrow = _voteEscrow;
	}
}

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L308-L311

function executeAddVoteEscrow() external onlyOwner {
	require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
	ve = VE(pendingVoteEscrow);
}

As the code below indicate, when ve corresponds to address(0), the Golom token amount for daily emission will always be dailyEmission or 600000 * 10**18, and staking reward will always be 0. In this case, calling the multiStakerClaim and stakerRewards will also revert so the staking rewards cannot be claimed or viewed. As a result, stakers do not get any staking rewards, and Golom token amount for daily emission and trading and exchange rewards are all calculated incorrectly.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L112-L114

uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
	rewardToken.totalSupply();
uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L172-L173

function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {
	require(address(ve) != address(0), ' VE not added yet');

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L215-L220

function stakerRewards(uint256 tokenid) public view returns (
		uint256,
		uint256,
		uint256[] memory
	){
	require(address(ve) != address(0), ' VE not added yet');

Proof of Concept

The following code can be added in test\GolomTrader.specs.ts to test the described scenario.

First, in the beforeEach block, the following code need to be added for setting up conditions that simulate reality.

beforeEach(async function () {
	...
	
	// simulate that reward token has some supply
	await golomToken.setMinter(await governance.getAddress());
	await golomToken.executeSetMinter();
	await golomToken.connect(governance).mint(rewardDistributor.address, 1);

	// set reward distributor as reward token's minter
	await golomToken.setMinter(rewardDistributor.address);
	await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);
	await golomToken.executeSetMinter();
});

The following test will pass to demonstrate the situation.

it('because vote escrow cannot be set in RewardDistributor, Golom token amount for daily emission is always 600000 * 10**18, and staking reward is always 0 and cannot be viewed or claimed', async () => {
	// initial vote escrow and pending vote escrow addresses are all address(0)
	expect(await rewardDistributor.connect(governance).ve()).to.be.equals("0x0000000000000000000000000000000000000000");
	expect(await rewardDistributor.connect(governance).pendingVoteEscrow()).to.be.equals("0x0000000000000000000000000000000000000000");

	// after calling addVoteEscrow, vote escrow and pending vote escrow addresses are still address(0)
	await rewardDistributor.connect(governance).addVoteEscrow(await governance.getAddress());
	expect(await rewardDistributor.connect(governance).ve()).to.be.equals("0x0000000000000000000000000000000000000000");
	expect(await rewardDistributor.connect(governance).pendingVoteEscrow()).to.be.equals("0x0000000000000000000000000000000000000000");

	await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);

	// after calling executeAddVoteEscrow, vote escrow and pending vote escrow addresses are still address(0)
	await rewardDistributor.connect(governance).executeAddVoteEscrow();
	expect(await rewardDistributor.connect(governance).ve()).to.be.equals("0x0000000000000000000000000000000000000000");
	expect(await rewardDistributor.connect(governance).pendingVoteEscrow()).to.be.equals("0x0000000000000000000000000000000000000000");

	// calling multiStakerClaim will revert
	await expect(
		rewardDistributor.connect(governance).multiStakerClaim([], [])
	).to.be.reverted;

	// calling stakerRewards will revert
	await expect(
		rewardDistributor.connect(governance).stakerRewards(0)
	).to.be.reverted;

	let exchangeAmount = ethers.utils.parseEther('1');
	let prePaymentAmt = ethers.utils.parseEther('0.25');
	let totalAmt = ethers.utils.parseEther('10');
	let tokenId = '0';

	const order = {
		collection: testErc1155.address,
		tokenId: tokenId,
		signer: await maker.getAddress(),
		orderType: 0,
		totalAmt: totalAmt,
		exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
		prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
		isERC721: false,
		tokenAmt: 5,
		refererrAmt: 0,
		root: '0x0000000000000000000000000000000000000000000000000000000000000000',
		reservedAddress: constants.AddressZero,
		nonce: 0,
		deadline: Date.now() + 100000,
		r: '',
		s: '',
		v: 0,
	};

	let signature = (await maker._signTypedData(domain, types, order)).substring(2);

	order.r = '0x' + signature.substring(0, 64);
	order.s = '0x' + signature.substring(64, 128);
	order.v = parseInt(signature.substring(128, 130), 16);

	const golomTokenTotalSupply0 = await golomToken.connect(maker).totalSupply();

	// call fillAsk for the first time
	await golomTrader.connect(taker).fillAsk(
		order,
		1,
		'0x0000000000000000000000000000000000000000',
		{
			paymentAmt: prePaymentAmt,
			paymentAddress: await governance.getAddress(),
		},
		receiver,
		{
			value: utils.parseEther('10.25'),
		}
	);

	// Golom tokens are increased by one daily emission
	const golomTokenTotalSupply1 = await golomToken.connect(maker).totalSupply();
	expect(golomTokenTotalSupply1.sub(golomTokenTotalSupply0)).to.be.equals(ethers.utils.parseEther('600000'));

	// staker reward is calculated to be 0
	expect(await rewardDistributor.connect(taker).rewardStaker(1)).to.be.equals("0");

	// call fillAsk for the second time
	await golomTrader.connect(taker).fillAsk(
		order,
		1,
		'0x0000000000000000000000000000000000000000',
		{
			paymentAmt: prePaymentAmt,
			paymentAddress: await governance.getAddress(),
		},
		receiver,
		{
			value: utils.parseEther('10.25'),
		}
	);

	// Golom tokens are still increased by one daily emission
	const golomTokenTotalSupply2 = await golomToken.connect(maker).totalSupply();
	expect(golomTokenTotalSupply2.sub(golomTokenTotalSupply1)).to.be.equals(ethers.utils.parseEther('600000'));

	// staker reward is still calculated to be 0
	expect(await rewardDistributor.connect(taker).rewardStaker(2)).to.be.equals("0");
});

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L298-L305 can be changed to the following code.

function addVoteEscrow(address _voteEscrow) external onlyOwner {
	if (address(ve) == address(0)) {
		ve = VE(_voteEscrow);
	} else {
		voteEscrowEnableDate = block.timestamp + 1 days;
		pendingVoteEscrow = _voteEscrow;
	}
}

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L381 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L389-L394 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L396-L399

Vulnerability details

Impact

In the _settleBalances function, the protocol fee is calculated as follows to take into account all amount that the taker wants to sell.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L381

uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;

For the cases where the bid or criteria bid order does and does not have a referrer, the ETH payments to the taker are calculated as follows, which multiply protocolfee by amount.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L389-L394

payEther(
	(o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
		amount -
		p.paymentAmt,
	msg.sender
);

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L396-L399

payEther(
	(o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
	msg.sender
);

When the order is for an ERC1155 with tokenAmt being larger than 1, calling the fillBid or fillCriteriaBid function, which further calls _settleBalances, with amount being larger than 1 will underpay the taker because the protocol fee already takes into account all amount, and multiplying protocolfee by amount again deducts too much incorrectly from the ETH payment to the taker. Since the taker is underpaid in this situation, the unused ETH, which should be paid to the taker, is locked in the GolomTrader contract without a way to withdraw it.

Proof of Concept

The following code can be added in test\GolomTrader.specs.ts to test the described scenario.

First, in the beforeEach block, the following code need to be added for setting up conditions that simulate reality.

beforeEach(async function () {
	...
	
	// simulate that reward token has some supply
	await golomToken.setMinter(await governance.getAddress());
	await golomToken.executeSetMinter();
	await golomToken.connect(governance).mint(rewardDistributor.address, 1);

	// set reward distributor as reward token's minter
	await golomToken.setMinter(rewardDistributor.address);
	await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);
	await golomToken.executeSetMinter();
});

The following test will pass to demonstrate the case for a bid order when there is no referrer. The cases when there is a referrer and for a criteria bid order are similar to this.

// this test requires forking mainnet and saving a copy of WETH9 contract deployed at 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2
it('Due to incorrect arithmetic operations, calling _settleBalances with amount input being larger than 1 underpays taker and locks unused ETH, which should be paid to taker, in GolomTrader for an ERC1155 bid order that is considered valid by validateOrder and has tokenAmt > 1', async () => {
	const orderMaker = taker;   // taker of this test suite creates the bid order
	const orderTaker = maker;   // maker of this test suite fills the bid order

	const wethInTrader = await ethers.getContractAt('WETH9', '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2');  // 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 is hardcoded WETH address in golomTrader
	await wethInTrader.connect(orderMaker).deposit({ value: ethers.utils.parseEther('100') });
	await wethInTrader.connect(orderMaker).approve(golomTrader.address, ethers.utils.parseEther('100'));

	let exchangeAmount = ethers.utils.parseEther('1');
	let prePaymentAmt = ethers.utils.parseEther('0.25');
	let totalAmt = ethers.utils.parseEther('10');
	let tokenId = '0';

	const order = {
		collection: testErc1155.address,
		tokenId: tokenId,
		signer: await orderMaker.getAddress(),
		orderType: 1,
		totalAmt: totalAmt,
		exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
		prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
		isERC721: false,
		tokenAmt: 5,    // tokenAmt is larger than 1
		refererrAmt: 0,
		root: '0x0000000000000000000000000000000000000000000000000000000000000000',
		reservedAddress: constants.AddressZero,
		nonce: 0,
		deadline: Date.now() + 100000,
		r: '',
		s: '',
		v: 0,
	};

	let signature = (await orderMaker._signTypedData(domain, types, order)).substring(2);

	order.r = '0x' + signature.substring(0, 64);
	order.s = '0x' + signature.substring(64, 128);
	order.v = parseInt(signature.substring(128, 130), 16);

	// this order is considered valid by validateOrder
	const [status, , ] = await golomTrader.connect(orderMaker).validateOrder(order);
	expect(status).to.be.equals("3");

	const takerEthBalanceBefore = await ethers.provider.getBalance(await orderTaker.getAddress());

	// calling fillBid will call _settleBalances
	await golomTrader.connect(orderTaker).fillBid(
		order,
		2,  // amount input is larger than 1
		'0x0000000000000000000000000000000000000000',
		{
			paymentAmt: 0,
			paymentAddress: await governance.getAddress(),
		},
		{
			gasLimit: 1e6
		}
	);

	const takerEthBalanceAfter = await ethers.provider.getBalance(await orderTaker.getAddress());
	const takerEthBalanceDiff = takerEthBalanceAfter.sub(takerEthBalanceBefore);

	const takerEthPaymentDeserved = ethers.utils.parseEther('17.4');  // ETH amount that should be paid to order taker
	const takerEthPaymentActual = ethers.utils.parseEther('17.3');    // ETH amount that is actually paid to order taker

	// order taker is underpaid
	expect(takerEthBalanceDiff).to.be.lt(takerEthPaymentDeserved);
	expect(takerEthPaymentActual.sub(takerEthBalanceDiff)).to.be.lt(ethers.utils.parseEther('0.000001'));   // 0.000001 ETH represents gas estimate for calling fillBid

	// golomTrader now has unused ETH that should be paid to order taker, and there is no way to withdraw it 
	expect(await ethers.provider.getBalance(golomTrader.address)).to.be.equals(takerEthPaymentDeserved.sub(takerEthPaymentActual));
});

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L381 can be changed to the following code so multiplication occurs before division.

uint256 protocolfee = (o.totalAmt * 50 * amount) / 10000;

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L389-L394 can be changed to the following code.

payEther(
	(o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
		amount -
		protocolfee -
		p.paymentAmt,
	msg.sender
);

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L396-L399 can be changed to the following code.

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/main/contracts/core/GolomTrader.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L269 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L384 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L402 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L100-L131

Vulnerability details

Impact

Although the documentation states that "Golom protocol has a native token which has a maximum supply limit of 1,000,000,000 (1 Billion $GOLOM tokens)", it is actually possible to mint over 1 billion $GOLOM tokens as indicated by the POC below.

When the total supply of $GOLOM tokens has exceeded 1 billion, trading orders by calling the fillAsk, fillBid, or fillCriteriaBid function will still send protocol fee in ETH to the RewardDistributor contract as indicated by the following code, where fillBid or fillCriteriaBid further calls the _settleBalances function. The addFee function of RewardDistributor will be called as well.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L242

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

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L269

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

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L384

payEther(protocolfee, address(distributor));

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L402

distributor.addFee([msg.sender, o.exchange.paymentAddress], protocolfee);

However, as indicated by the following code, because addFee of RewardDistributor returns before calling the weth.deposit function, the paid fee in ETH is locked in RewardDistributor without other ways to withdraw it in this situation.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L100-L131

if (rewardToken.totalSupply() > 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
if (block.timestamp > startTime + (epoch) * secsInDay) {
	...
	if (previousEpochFee > 0) {
		if (epoch == 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}();
		}
	}

Proof of Concept

The documentation indicates that 687,500,000 $GOLOM tokens will be used for the staking, trading, and exchange rewards and the daily emission would be 600,000. Since 687,500,000 / 600,000 = 1,145.833333..., which is not a whole number, minting over 1 billion $GOLOM tokens can happen on the 1146th day after the daily emission starts given that _maxSupply is type(uint224).max for the GolomToken contract.

The following code can be added in test\GolomTrader.specs.ts to test the described scenarios.

First, in the beforeEach block, the following code need to be added for setting up conditions that simulate reality.

beforeEach(async function () {
	...
	
	// simulate that reward token has some supply
	await golomToken.setMinter(await governance.getAddress());
	await golomToken.executeSetMinter();
	await golomToken.connect(governance).mint(rewardDistributor.address, 1);

	// set reward distributor as reward token's minter
	await golomToken.setMinter(rewardDistributor.address);
	await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);
	await golomToken.executeSetMinter();
});

The following test will pass to demonstrate the case where $GOLOM tokens is minted for over 1 billion and fillAsk is called afterwards.

it('calling fillAsk will lock fee in ETH in RewardDistributor for an ask order that is considered valid by validateOrder when total supply of reward token has exceeded 1000000000 * 10**18', async () => {
	// simulate a situation where reward token's total supply exceeds 1000000000 * 10**18
	await golomToken.setMinter(await governance.getAddress());
	await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);
	await golomToken.executeSetMinter();
	await golomToken.connect(governance).mint(rewardDistributor.address, ethers.utils.parseEther('2000000000'));

	// reward token's total supply does exceed 1000000000 * 10**18 before processing the ask order
	expect(await golomToken.connect(governance).totalSupply()).to.be.gt(ethers.utils.parseEther('1000000000'));

	await golomToken.setMinter(rewardDistributor.address);
	await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);
	await golomToken.executeSetMinter();

	let exchangeAmount = ethers.utils.parseEther('1');
	let prePaymentAmt = ethers.utils.parseEther('0.25');
	let totalAmt = ethers.utils.parseEther('10');
	let tokenId = await testErc721.current();

	const order = {
		collection: testErc721.address,
		tokenId: tokenId,
		signer: await maker.getAddress(),
		orderType: 0,
		totalAmt: totalAmt,
		exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
		prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
		isERC721: true,
		tokenAmt: 1,
		refererrAmt: 0,
		root: '0x0000000000000000000000000000000000000000000000000000000000000000',
		reservedAddress: constants.AddressZero,
		nonce: 0,
		deadline: Date.now() + 100000,
		r: '',
		s: '',
		v: 0,
	};

	let signature = (await maker._signTypedData(domain, types, order)).substring(2);

	order.r = '0x' + signature.substring(0, 64);
	order.s = '0x' + signature.substring(64, 128);
	order.v = parseInt(signature.substring(128, 130), 16);

	// this order is considered valid by validateOrder
	const [status, , ] = await golomTrader.connect(maker).validateOrder(order);
	expect(status).to.be.equals("3");

	await golomTrader.connect(taker).fillAsk(
		order,
		1,
		'0x0000000000000000000000000000000000000000',
		{
			paymentAmt: prePaymentAmt,
			paymentAddress: await governance.getAddress(),
		},
		receiver,
		{
			value: utils.parseEther('10.25'),
		}
	);

	// fee in ETH is received by rewardDistributor but there is no way to withdraw it
	expect(await ethers.provider.getBalance(rewardDistributor.address)).to.be.equals(ethers.utils.parseEther('0.05'));
});

The following test will pass to demonstrate the case where $GOLOM tokens is minted for over 1 billion and fillBid is called afterwards. The case where fillCriteriaBid is called instead is similar to this.

// this test requires forking mainnet and saving a copy of WETH9 contract deployed at 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2
it('calling _settleBalances will lock fee in ETH in RewardDistributor for a bid order that is considered valid by validateOrder when total supply of reward token has exceeded 1000000000 * 10**18', async () => {
	// simulate a situation where reward token's total supply exceeds 1000000000 * 10**18
	await golomToken.setMinter(await governance.getAddress());
	await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);
	await golomToken.executeSetMinter();
	await golomToken.connect(governance).mint(rewardDistributor.address, ethers.utils.parseEther('2000000000'));

	// reward token's total supply does exceed 1000000000 * 10**18 before processing the ask order
	expect(await golomToken.connect(governance).totalSupply()).to.be.gt(ethers.utils.parseEther('1000000000'));

	await golomToken.setMinter(rewardDistributor.address);
	await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);
	await golomToken.executeSetMinter();

	const orderMaker = taker;   // taker of this test suite creates the bid order
	const orderTaker = maker;   // maker of this test suite fills the bid order

	const wethInTrader = await ethers.getContractAt('WETH9', '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2');  // 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 is hardcoded WETH address in golomTrader
	await wethInTrader.connect(orderMaker).deposit({ value: ethers.utils.parseEther('100') });
	await wethInTrader.connect(orderMaker).approve(golomTrader.address, ethers.utils.parseEther('100'));

	let exchangeAmount = ethers.utils.parseEther('1');
	let prePaymentAmt = ethers.utils.parseEther('0.25');
	let totalAmt = ethers.utils.parseEther('10');
	let tokenId = await testErc721.current();

	const order = {
		collection: testErc721.address,
		tokenId: tokenId,
		signer: await orderMaker.getAddress(),
		orderType: 1,
		totalAmt: totalAmt,
		exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
		prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
		isERC721: true,
		tokenAmt: 1,
		refererrAmt: 0,
		root: '0x0000000000000000000000000000000000000000000000000000000000000000',
		reservedAddress: constants.AddressZero,
		nonce: 0,
		deadline: Date.now() + 100000,
		r: '',
		s: '',
		v: 0,
	};

	let signature = (await orderMaker._signTypedData(domain, types, order)).substring(2);

	order.r = '0x' + signature.substring(0, 64);
	order.s = '0x' + signature.substring(64, 128);
	order.v = parseInt(signature.substring(128, 130), 16);

	// this order is considered valid by validateOrder
	const [status, , ] = await golomTrader.connect(maker).validateOrder(order);
	expect(status).to.be.equals("3");

	// calling fillBid will call _settleBalances
	await golomTrader.connect(orderTaker).fillBid(
		order,
		1,
		'0x0000000000000000000000000000000000000000',
		{
			paymentAmt: prePaymentAmt,
			paymentAddress: await governance.getAddress(),
		},
		{
			gasLimit: 1e6
		}
	);

	// fee in ETH is received by rewardDistributor but there is no way to withdraw it
	expect(await ethers.provider.getBalance(rewardDistributor.address)).to.be.equals(ethers.utils.parseEther('0.05'));
});

Tools Used

VSCode

Recommended Mitigation Steps

fillAsk and _settleBalances can be changed to not charge any protocol fees, not transfer any ETH to RewardDistributor, and not calling addFee of RewardDistributor when the total supply of $GOLOM tokens has exceeded 1 billion because the 687,500,000 $GOLOM tokens to be used for the staking, trading, and exchange rewards should all be minted already.

Moreover, if willing to be more restrictive, the GolomToken contract can be updated to have an upper total supply limit of 1000000000 * 10**18 but the actual $GOLOM token amount being minted at maximum could be less than that.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L151-L156

Vulnerability details

Impact

When the payEther function of the GolomTrader contract is called, the transfer function is used to transfer ETH. This function forwards a fixed 2300 gas. If the receiver is a contract that consumes more than 2300 gas for processing after receiving the ETH, the transaction will revert.

Proof of Concept

When calling the fillAsk, fillBid, or fillCriteriaBid function, payEther is eventually called as follows for sending ETH to maker, taker, referrer, exchange, extra payment receiver, etc.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L151-L156

function payEther(uint256 payAmt, address payAddress) internal {
	if (payAmt > 0) {
		// if royalty has to be paid
		payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
	}
}

It is possible that the receiver is a contract that needs more than 2300 gas to execute more logics after receiving the ETH. In this case, the transaction will revert because the forwarded gas deplete.

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L151-L156 can be updated to the following code.

function payEther(uint256 payAmt, address payAddress) internal {
	if (payAmt > 0) {
		(bool success, ) = payable(payAddress).call{value: payAmt}("");
		require(success, "ETH transfer failed");
	}
}

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L236 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L301 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L361

Vulnerability details

Impact

When calling the fillAsk, fillBid, or fillCriteriaBid function of the GolomTrader contract, the ERC721 transferFrom function is used to transfer the ERC721 token. If the receiver is a contract that does not support the ERC721 protocol, the token will be locked and cannot be retrieved.

For reference, OpenZeppelin's documentation for transferFrom states: "Usage of this method is discouraged, use safeTransferFrom whenever possible."

Proof of Concept

For an order with collection being an ERC721 address and isERC721 being true, the following code are executed when calling fillAsk, fillBid, or fillCriteriaBid.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L236

ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L301

nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L361

nftcontract.transferFrom(msg.sender, o.signer, tokenId);

Because calling transferFrom of OpenZeppelin's ERC721 contract does not execute the _checkOnERC721Received function, it is unknown if the receiving contract inherits from the IERC721Receiver interface and has the onERC721Received function or not. It is possible that the receiving contract does not support the ERC721 protocol, which causes the transferred ERC721 token to be locked.

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L236 can be changed to the following code.

ERC721(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId);

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L301 can be changed to the following code.

nftcontract.safeTransferFrom(msg.sender, o.signer, o.tokenId);

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L361 can be changed to the following code.

nftcontract.safeTransferFrom(msg.sender, o.signer, tokenId);

Judge has assessed an item in Issue #889 as Medium risk. The relevant finding follows:

[L-05] safeTransferFrom of VoteEscrowCore does not fully comply ERC721 standard Per https://eips.ethereum.org/EIPS/eip-721, safeTransferFrom needs to throw if the receiver is a contract that does not have the onERC721Received function that returns bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")). Based on the following code, the safeTransferFrom function of VoteEscrowCore does check if the receiving contract has onERC721Received but does not check if that function returns bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")). To comply the standard, this check needs to be added.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L594-L616

function safeTransferFrom(
	address _from,
	address _to,
	uint256 _tokenId,
	bytes memory _data
) public {
	_transferFrom(_from, _to, _tokenId, msg.sender);

	if (_isContract(_to)) {
		// Throws if transfer destination is a contract which does not implement 'onERC721Received'
		try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4) {} catch (
			bytes memory reason
		) {
			if (reason.length == 0) {
				revert('ERC721: transfer to non ERC721Receiver implementer');
			} else {
				assembly {
					revert(add(32, reason), mload(reason))
				}
			}
		}
	}
}

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L217

Vulnerability details

Impact

When calling the fillAsk function of the GolomTrader contract, the taker can pay more ETH than required for an ask order because of the >= comparison in the following code.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L217

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

When too much ETH is paid, the extra ETH is locked in GolomTrader since there is no other way to withdraw the extra amount.

Proof of Concept

The following code can be added in test\GolomTrader.specs.ts to test the described scenario.

First, in the beforeEach block, the following code need to be added for setting up conditions that simulate reality.

beforeEach(async function () {
	...
	
	// simulate that reward token has some supply
	await golomToken.setMinter(await governance.getAddress());
	await golomToken.executeSetMinter();
	await golomToken.connect(governance).mint(rewardDistributor.address, 1);

	// set reward distributor as reward token's minter
	await golomToken.setMinter(rewardDistributor.address);
	await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);
	await golomToken.executeSetMinter();
});

The following test will pass to demonstrate the situation.

it('extra msg.value will be locked in GolomTrader contract if too much msg.value are paid when calling fillAsk', async () => {
	let exchangeAmount = ethers.utils.parseEther('1');
	let prePaymentAmt = ethers.utils.parseEther('0.25');
	let totalAmt = ethers.utils.parseEther('10');
	let tokenId = await testErc721.current();

	const order = {
		collection: testErc721.address,
		tokenId: tokenId,
		signer: await maker.getAddress(),
		orderType: 0,
		totalAmt: totalAmt,
		exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
		prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
		isERC721: true,
		tokenAmt: 1,
		refererrAmt: 0,
		root: '0x0000000000000000000000000000000000000000000000000000000000000000',
		reservedAddress: constants.AddressZero,
		nonce: 0,
		deadline: Date.now() + 100000,
		r: '',
		s: '',
		v: 0,
	};

	let signature = (await maker._signTypedData(domain, types, order)).substring(2);

	order.r = '0x' + signature.substring(0, 64);
	order.s = '0x' + signature.substring(64, 128);
	order.v = parseInt(signature.substring(128, 130), 16);

	await golomTrader.connect(taker).fillAsk(
		order,
		1,
		'0x0000000000000000000000000000000000000000',
		{
			paymentAmt: prePaymentAmt,
			paymentAddress: await governance.getAddress(),
		},
		receiver,
		{
			value: utils.parseEther('11.25'),   // msg.value is 11.25
		}
	);

	// 11.25 ETH are paid when calling fillAsk but only 10.25 ETH are paid to relevant parties afterwards
	expect(await ethers.provider.getBalance(golomTrader.address)).to.be.equals(utils.parseEther('1'));
});

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L217 can be changed to the following code.

require(msg.value == o.totalAmt * amount + p.paymentAmt, 'mgmtm');

[L-01] Order Status 0 cannot be returned for validateOrder

Based on the following code, when signaturesigner != o.signer is true, require(signaturesigner == o.signer, 'invalid signature'); will revert. This means that (0, hashStruct, 0) can never be returned. If Order Status 0 has a purpose and needs to be returned, require(signaturesigner == o.signer, 'invalid signature'); needs to be removed.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L164-L194

function validateOrder(Order calldata o)
	public
	view
	returns (
		uint256,
		bytes32,
		uint256
	)
{
	// match signature
	bytes32 hashStruct = _hashOrder(o);
	bytes32 hash = keccak256(abi.encodePacked('\x19\x01', EIP712_DOMAIN_TYPEHASH, hashStruct));
	address signaturesigner = ecrecover(hash, o.v, o.r, o.s);
	require(signaturesigner == o.signer, 'invalid signature');
	if (signaturesigner != o.signer) {
		return (0, hashStruct, 0);
	}
	//deadline
	if (block.timestamp > o.deadline) {
		return (1, hashStruct, 0);
	}
	// not cancelled by nonce or by hash
	if (o.nonce != nonces[o.signer]) {
		return (2, hashStruct, 0);
	}
	if (filled[hashStruct] >= o.tokenAmt) {
		// handles erc1155
		return (2, hashStruct, 0);
	}
	return (3, hashStruct, o.tokenAmt - filled[hashStruct]);
}

[L-02] startTime in RewardDistributor is not 31st July 2022 20:00 UTC

The documentation indicates that the Genesis Period ends on 07/31/2022 20:00 UTC, and the daily emissions and staking, trading, and exchange rewards start afterwards. However, startTime in the RewardDistributor contract is 07/30/2022 20:00 UTC. Based on the following addFee code, daily emission and rewards can already start on 07/30/2022 20:01 UTC when block.timestamp > startTime + (epoch) * secsInDay becomes true, which is one day earlier than 07/31/2022 20:00 UTC. Either the code or the documentation needs to be adjusted accordingly to avoid user confusion and accounting discrepancies.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L98-L138

function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
	//console.log(block.timestamp,epoch,fee);
	if (rewardToken.totalSupply() > 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
	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();
		// 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;
		rewardToken.mint(address(this), tokenToEmit);
		epochBeginTime[epoch] = block.number;
		if (previousEpochFee > 0) {
			if (epoch == 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);
	}
	feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee;
	feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee;
	epochTotalFee[epoch] = epochTotalFee[epoch] + fee;
	return;
}

[L-03] Taker has incentive to use self as referrer when calling fillAsk

Even when there is no referrer, the taker can make herself or himself as the referrer for an ask order when calling fillAsk in order to pay less to the maker. To disincentivize this behavior, https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L250 can be changed to the following code.

if (o.refererrAmt > 0 && referrer != address(0) && referrer != msg.sender) {

[L-04] When an order is already expired, there is no need to cancel it

Cancelling an expired order is unnecessary and wastes gas. Hence, https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L312-L317 can be changed to the following code.

function cancelOrder(Order calldata o) public nonReentrant {
	require(o.signer == msg.sender && block.timestamp <= o.deadline, "order cannot be cancelled");
	(, bytes32 hashStruct, ) = validateOrder(o);
	filled[hashStruct] = o.tokenAmt + 1;
	emit OrderCancelled(hashStruct);
}

[L-05] safeTransferFrom of VoteEscrowCore does not fully comply ERC721 standard

Per https://eips.ethereum.org/EIPS/eip-721, safeTransferFrom needs to throw if the receiver is a contract that does not have the onERC721Received function that returns bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")). Based on the following code, the safeTransferFrom function of VoteEscrowCore does check if the receiving contract has onERC721Received but does not check if that function returns bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")). To comply the standard, this check needs to be added.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L594-L616

function safeTransferFrom(
	address _from,
	address _to,
	uint256 _tokenId,
	bytes memory _data
) public {
	_transferFrom(_from, _to, _tokenId, msg.sender);

	if (_isContract(_to)) {
		// Throws if transfer destination is a contract which does not implement 'onERC721Received'
		try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4) {} catch (
			bytes memory reason
		) {
			if (reason.length == 0) {
				revert('ERC721: transfer to non ERC721Receiver implementer');
			} else {
				assembly {
					revert(add(32, reason), mload(reason))
				}
			}
		}
	}
}

[L-06] Documentation are needed for time locks for setting critical components

After the critical components, such as distributor in the GolomTrader contract, are initialized, setting them again, such as by using the setDistributor function, often require a 1-day time lock. This practice needs to be documented to avoid any confusion for the users.

[L-07] WETH address is hardcoded in GolomTrader but is set in RewardDistributor

It can be confusing that the WETH address is hardcoded in GolomTrader but is set in RewardDistributor. If there is a need for RewardDistributor to use a different WETH contract than GolomTrader does, this needs to be documented and explained to the users. Otherwise, the approaches for accessing the WETH contract need to be consistent across both contracts.

[L-08] UNRESOLVED COMMENTS

Comments like todos or questions indicate that there are unresolved action items for implementation, which need to be addressed before protocol deployment.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L107

// this assumes atleast 1 trade is done daily??????

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L68

/// @notice Explain to an end user what this does

[L-09] Require() should be used instead of assert()

assert() should be used for testing invariants and internal bugs. require() should be used to check function call returns, function inputs, valid conditions, etc. The following assert() can be changed to require().

vote-escrow\VoteEscrowCore.sol
  493:         assert(idToOwner[_tokenId] == address(0));  
  506:         assert(idToOwner[_tokenId] == _from);  
  519:         assert(idToOwner[_tokenId] == _owner);  
  666:         assert(_operator != msg.sender);  
  679:         assert(_to != address(0));  
  861:         assert(IERC20(token).transferFrom(from, address(this), _value));    
  977:         assert(_isApprovedOrOwner(msg.sender, _tokenId));  
  981:         assert(_value > 0); // dev: need non-zero value  
  991:         assert(_isApprovedOrOwner(msg.sender, _tokenId));  
  1007:        assert(_isApprovedOrOwner(msg.sender, _tokenId));  
  1023:        assert(IERC20(token).transfer(msg.sender, value));  
  1110:        assert(_block <= block.number);  
  1206:        assert(_block <= block.number);  

[N-01] Constants can be used instead of magic numbers

To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing the magic numbers in the following code with constants.

core\GolomTrader.sol
  212:         o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,  
  242:         payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));
  263:         (o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount,  
  269:         distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);  
  381:         uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;  
  449:         distributorEnableDate = block.timestamp + 1 days;

governance\GolomToken.sol
  44:          _mint(_airdrop, 150_000_000 * 1e18); 
  52:          _mint(_rewardDistributor, 62_500_000 * 1e18); 
  60:          minterEnableDate = block.timestamp + 1 days;  

rewards\RewardDistributor.sol
  84:          startTime = 1659211200; 
  100:         if (rewardToken.totalSupply() > 1000000000 * 10**18) {  
  120:         rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;  
  121:         rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;  
  286:         traderEnableDate = block.timestamp + 1 days; 
  302:         voteEscrowEnableDate = block.timestamp + 1 days; 

vote-escrow\TokenUriHelper.sol
  17:          uint256 encodedLen = 4 * ((len + 2) / 3); 
  20:          bytes memory result = new bytes(encodedLen + 32);  
  25:          let tablePtr := add(table, 1)  
  26:          let resultPtr := add(result, 32)  
  139:         temp /= 10; 
  144:         buffer[digits] = bytes1(uint8(48 + uint256(value % 10)));  
  145:         value /= 10;  

vote-escrow\VoteEscrowCore.sol
  1044:        for (uint256 i = 0; i < 128; ++i) { 
  1115:        for (uint256 i = 0; i < 128; ++i) {  

[N-02] Missing reason strings in require()

When the reason strings are missing in the require statements, it is unclear about why certain conditions revert. Please add descriptive reason strings for the following require and revert statements.

core\GolomTrader.sol
  220:         require(msg.sender == o.reservedAddress);
  291:         require(msg.sender == o.reservedAddress);    
  293:         require(o.orderType == 1);
  295:         require(status == 3); 
  296:         require(amountRemaining >= amount);  
  345:         require(msg.sender == o.reservedAddress);    
  347:         require(o.orderType == 2);    
  349:         require(status == 3);    
  350:         require(amountRemaining >= amount);    

rewards\RewardDistributor.sol
  88:          require(msg.sender == trader);    
  144:         require(epochs[index] < epoch);    
  158:         require(epochs[index] < epoch);    

vote-escrow\VoteEscrowCore.sol
  360:         require(_entered_state == _not_entered);    
  540:         require(_isApprovedOrOwner(_sender, _tokenId));    
  646:         require(owner != address(0));    
  648:         require(_approved != owner);    
  652:         require(senderIsOwner || senderIsApprovedForAll);    
  869:         require(msg.sender == voter);    
  874:         require(msg.sender == voter);    
  879:         require(msg.sender == voter);    
  884:         require(msg.sender == voter);    
  889:         require(msg.sender == voter);    
  895:         require(_from != _to);    
  896:         require(_isApprovedOrOwner(msg.sender, _from));    
  897:         require(_isApprovedOrOwner(msg.sender, _to));    
  927:         require(_value > 0); // dev: need non-zero value    
  944:         require(_value > 0); // dev: need non-zero value    

[N-03] Commented out code

Commented out code can cause confusion. If the following code are not used anymore, please remove them for readability and maintainability.

rewards\RewardDistributor.sol
  99:          //console.log(block.timestamp,epoch,fee);

vote-escrow\VoteEscrowDelegation.sol
  6:           // import {Math} from '@openzeppelin-contracts/utils/math/SafeCast.sol';
  218:         // /// @notice Remove delegation by user

[G-01] UNUSED PACKAGE SHOULD BE REMOVED

hardhat/console.sol, which is used for development, should be removed before deployment to reduce code size and deployment gas.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L9

import 'hardhat/console.sol';

[G-02] STRUCT VARIABLES CAN BE PACKED INTO SMALLER NUMBER OF STORAGE SLOTS

Positions of some struct variables can be changed so the adjacent variables can occupy the same storage slot to save space and gas.

For the following structs, isERC721 can be moved to be after root and before reservedAddress so it can share the same storage slot with reservedAddress.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/Emitter.sol#L6-L24

    struct Order {
        address collection; // NFT contract address
        uint256 tokenId; // order for which tokenId of the collection
        address signer; // maker of order address
        uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root
        uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount
        Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt
        Payment prePayment; // another payment , can be used for royalty, facilating trades
        bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
        uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
        uint256 refererrAmt; // amt to pay to the address that helps in filling your order
        bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
        address reservedAddress; // if not address(0) , only this address can fill the order
        uint256 nonce; // nonce of order usefull for cancelling in bulk
        uint256 deadline; // timestamp till order is valid epoch timestamp in secs
        uint8 v;
        bytes32 r;
        bytes32 s;
    }

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L47-L65

    struct Order {
        address collection; // NFT contract address
        uint256 tokenId; // order for which tokenId of the collection
        address signer; // maker of order address
        uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root
        uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount
        Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt
        Payment prePayment; // another payment , can be used for royalty, facilating trades
        bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
        uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
        uint256 refererrAmt; // amt to pay to the address that helps in filling your order
        bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
        address reservedAddress; // if not address(0) , only this address can fill the order
        uint256 nonce; // nonce of order usefull for cancelling in bulk
        uint256 deadline; // timestamp till order is valid epoch timestamp in secs
        uint8 v;
        bytes32 r;
        bytes32 s;
    }

[G-03] REVERT REASON STRINGS CAN BE SHORTENED TO FIT IN 32 BYTES

Revert reason strings that are longer than 32 bytes need more memory space and more mstore operation(s) than these that are shorter than 32 bytes when reverting. Please consider shortening the following revert reason strings.

governance\GolomToken.sol
  24:          require(msg.sender == minter, 'GolomToken: only reward distributor can enable');   

rewards\RewardDistributor.sol
  181:         require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');   
  292:         require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');   
  309:         require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');   

vote-escrow\VoteEscrowCore.sol
  608:         revert('ERC721: transfer to non ERC721Receiver implementer');   
  929:         require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');   
  945:         require(unlock_time > block.timestamp, 'Can only lock until time in the future');   
  983:         require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');   

vote-escrow\VoteEscrowDelegation.sol
  73:         require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');   

[G-04] LOOP COUNTER VARIABLE DOES NOT ALWAYS NEED TO START FROM 0

Because there are no trading and exchange rewards for Epoch 0, index can start from 1 in the following for loops. This saves gas for one iteration for each for loop.

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L254-L265

    function traderRewards(address addr) public view returns (
            uint256        
            ){
        uint256 reward = 0;
        for (uint256 index = 0; index < epoch; index++) {
            reward =
                reward +
                (rewardTrader[index] * feesTrader[addr][index]) /
                epochTotalFee[index];
        }
        return (reward);
    }

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L269-L280

    function exchangeRewards(address addr) public view returns (
            uint256
        ){
        uint256 reward = 0;
        for (uint256 index = 0; index < epoch; index++) {
            reward =
                reward +
                (rewardExchange[index] * feesExchange[addr][index]) /
                epochTotalFee[index];
        }
        return (reward);
    }

[G-05] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it. For example, uint256 i can be used instead of uint256 i = 0 in the following code.

core\GolomTrader.sol
  415:         for (uint256 i = 0; i < proof.length; i++) {

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

vote-escrow\VoteEscrowCore.sol
  745:         for (uint256 i = 0; i < 255; ++i) {
  1044:        for (uint256 i = 0; i < 128; ++i) { 
  1115:        for (uint256 i = 0; i < 128; ++i) {  
  1167:        for (uint256 i = 0; i < 255; ++i) {

vote-escrow\VoteEscrowDelegation.sol
  171:         for (uint256 index = 0; index < delegated.length; index++) {
  189:         for (uint256 index = 0; index < delegatednft.length; index++) {

[G-06] ARRAY LENGTH CAN BE CACHED OUTSIDE OF LOOP

Caching the array length outside of the loop and using the cached length in the loop costs less gas than reading the array length for each iteration. For example, proof.length in the following code can be cached outside of the loop like uint256 proofLen = proof.length, and i < proofLen can be used for each iteration.

core\GolomTrader.sol
  415:         for (uint256 i = 0; i < proof.length; i++) {

rewards\RewardDistributor.sol
  143:         for (uint256 index = 0; index < epochs.length; index++) {   
  157:         for (uint256 index = 0; index < epochs.length; index++) {   
  180:         for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {   
  183:         for (uint256 index = 0; index < epochs.length; index++) {
  
vote-escrow\VoteEscrowDelegation.sol
  171:         for (uint256 index = 0; index < delegated.length; index++) {
  189:         for (uint256 index = 0; index < delegatednft.length; index++) {
  199:         for (uint256 i; i < _array.length; i++) {
  

[G-07] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++. For example, i++ can be changed to ++i in the following code.

core\GolomTrader.sol
  415:         for (uint256 i = 0; i < proof.length; i++) {

rewards\RewardDistributor.sol
  143:         for (uint256 index = 0; index < epochs.length; index++) {   
  157:         for (uint256 index = 0; index < epochs.length; index++) {   
  180:         for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {   
  183:         for (uint256 index = 0; index < epochs.length; index++) {  
  226:         for (uint256 index = 0; index < epoch; index++) {
  258:         for (uint256 index = 0; index < epoch; index++) {   
  273:         for (uint256 index = 0; index < epoch; index++) {
  
vote-escrow\VoteEscrowDelegation.sol
  171:         for (uint256 index = 0; index < delegated.length; index++) {
  189:         for (uint256 index = 0; index < delegatednft.length; index++) {
  199:         for (uint256 i; i < _array.length; i++) {

[G-08] ARITHMETIC OPERATIONS THAT DO NOT OVERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not overflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

For the following loops, if increasing the counter variable is very unlikely to overflow, then unchecked {++i} at the end of the loop block can be used, where i is the counter variable.

core\GolomTrader.sol
  415:         for (uint256 i = 0; i < proof.length; i++) {

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

vote-escrow\VoteEscrowCore.sol
  745:         for (uint256 i = 0; i < 255; ++i) {
  1044:        for (uint256 i = 0; i < 128; ++i) { 
  1115:        for (uint256 i = 0; i < 128; ++i) {  
  1167:        for (uint256 i = 0; i < 255; ++i) {

vote-escrow\VoteEscrowDelegation.sol
  171:         for (uint256 index = 0; index < delegated.length; index++) {
  189:         for (uint256 index = 0; index < delegatednft.length; index++) {
  199:         for (uint256 i; i < _array.length; i++) {