Golom contest - GalloDaSballo's results

Lines of code

https://github.com/GalloDaSballo/brownie-starter/blob/28f708ebd8cfb4fa1a412df9fa376d33813f281f/contracts/VoteEscrowDelegation.sol#L87-L105

Vulnerability details

The function _writeCheckpoint performs: checkpoints[toTokenId][nCheckpoints - 1]; which, on first checkpoint will always revert as the value nCheckpoints will be zero

https://github.com/GalloDaSballo/brownie-starter/blob/28f708ebd8cfb4fa1a412df9fa376d33813f281f/contracts/VoteEscrowDelegation.sol#L87-L105

    /**
     * @notice Writes the checkpoint to store current NFTs in the specific block
     */
    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;
        }
    }

This is because nCheckpoints on first checkpoint will always be zero and on solidity >0.8 that will cause a revert

Remediation Steps

Add a extra case to handle 0 nCheckpoints.

I've rewritten and ran the following code which seems to work fine

Rewrite to

    /**
     * @notice Writes the checkpoint to store current NFTs in the specific block
     */
    function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {
        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

        if (nCheckpoints > 0) {

            Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
            
            if (oldCheckpoint.fromBlock == block.number){
                oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
                return; // End Early
            }
        }

        checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
        numCheckpoints[toTokenId] = nCheckpoints + 1;
    }

Lines of code

https://github.com/GalloDaSballo/brownie-starter/blob/28f708ebd8cfb4fa1a412df9fa376d33813f281f/contracts/VoteEscrowDelegation.sol#L67-L76

Vulnerability details

NOTE: This finding requires fixing another bug in the codebase:

• Fix underflow on first checkpoint, see: "delegate and removeDelegation are broken due to _writeCheckpoint underflow"

Once the code is fixed as below, (handing of first checkpoint), another vulnerability will emerge

https://github.com/GalloDaSballo/brownie-starter/blob/28f708ebd8cfb4fa1a412df9fa376d33813f281f/contracts/VoteEscrowDelegation.sol#L67-L76

    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];

        if (nCheckpoints > 0) {
            Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
            checkpoint.delegatedTokenIds.push(tokenId);

Because delegate does: checkpoint.delegatedTokenIds.push(tokenId); then any new delegated token will be added to the list, allowing users to self-delegate, delegate to other people and ultimately to inflate their voting power infinitely.

Note about POC:

The POC requires fixing a bug in _writeCheckpoint, see my report: "delegate and removeDelegation are broken due to _writeCheckpoint underflow"

To bring further clarity to the POC I've changed _getCurrentDelegated to be a public function so we can see the internal data structure for delegation

I've also changed _deposit_for to have the transfer commented to avoid needless setup

POC

Launching 'ganache-cli --accounts 10 --hardfork istanbul --fork https://mainnet.infura.io/v3/uFEqPEkn09U --gasLimit 12000000 --mnemonic brownie --port 8545 --chainId 1'...
Brownie environment is ready.

## Deploy Lib
>>> t = TokenUriHelper.deploy({"from": a[0]})
Transaction sent: 0x8743a7e9b2fff2e69d14f76fdce12588fb8c5c648f1ee5e6f38227dd4f873320
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 4
  TokenUriHelper.constructor confirmed   Block: 15246511   Gas used: 2092588 (17.44%)
  TokenUriHelper deployed at: 0xe0aA552A10d7EC8760Fc6c246D391E698a82dDf9

## And Deploy Contract
>>> x = VoteEscrow.deploy(ETH_ADDRESS, {"from": a[0]})
Transaction sent: 0x094a75c8868c5b94cfa511898953b0802251518b4bd9fbe73855d362c82ce476
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 5
  VoteEscrow.constructor confirmed   Block: 15246512   Gas used: 3865003 (32.21%)
  VoteEscrow deployed at: 0x6b4BDe1086912A6Cb24ce3dB43b3466e6c72AFd3

## Create Lock Id 1 || Edited to require no tokens || Just comment the transfer
>>> x.create_lock(123, chain.time() / 1000, {"from": a[0]})
Transaction sent: 0xb58b326e54e9d08079abf3c527882a975d2abd0e755efb1c63f9b191196d40ee
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 6
  VoteEscrow.create_lock confirmed   Block: 15246513   Gas used: 314838 (2.62%)

<Transaction '0xb58b326e54e9d08079abf3c527882a975d2abd0e755efb1c63f9b191196d40ee'>

## Create Lock Id 2
>>> x.create_lock(123, chain.time() / 1000, {"from": a[1]})
Transaction sent: 0x6fe2dbc56b3bd130ccf681415390968f6b5e8a105fe2f856ea4b274f6b1bf286
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 5
  VoteEscrow.create_lock confirmed   Block: 15246514   Gas used: 261993 (2.18%)

<Transaction '0x6fe2dbc56b3bd130ccf681415390968f6b5e8a105fe2f856ea4b274f6b1bf286'>

## Delegate 1 to 2
>>> x.delegate(1, 2, {"from": a[0]})
Transaction sent: 0x3ecbc209b9ad674286288fa4260a3df700241d3d3cc3f9fdda214b8ace4a96b9
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 7
  VoteEscrow.delegate confirmed   Block: 15246515   Gas used: 134655 (1.12%)

<Transaction '0x3ecbc209b9ad674286288fa4260a3df700241d3d3cc3f9fdda214b8ace4a96b9'>

## Delegate 1 to 1 (to itself)
>>> x.delegate(1, 1, {"from": a[0]})
Transaction sent: 0xdd9e30473694cc67fd44f98fb2ed3581b3ba335c2148d4f1a2a3c50ec74419b2
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 8
  VoteEscrow.delegate confirmed   Block: 15246516   Gas used: 119655 (1.00%)

<Transaction '0xdd9e30473694cc67fd44f98fb2ed3581b3ba335c2148d4f1a2a3c50ec74419b2'>

## Notice duplicate delegated 1 has delegated to themselves
>>> x._getCurrentDelegated(1)
(1)

## Notice duplicate delegated 1 has delegated to 2
>>> x._getCurrentDelegated(2)
(1)

## Increase lock to get some voting power
>>> x.increase_amount(1, 1e18, {"from": a[0]})
Transaction sent: 0xf757aa367cda95901946093468bbd6cdefe1a960e32b008a9a179f6f590a5934
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 12
  VoteEscrow.increase_amount confirmed   Block: 15246520   Gas used: 195232 (1.63%)

<Transaction '0xf757aa367cda95901946093468bbd6cdefe1a960e32b008a9a179f6f590a5934'>

## This is the voting power of 1, all good so far
>>> x.getVotes(1)
12367556124863535

## 2 also received 1s voting power (2 total amount is 123 wei which is negligible)
>>> x.getVotes(2)
12367556124863535

## === Infinite votes POC === ##
>>> x.delegate(1, 1, {"from": a[0]})
Transaction sent: 0x1dc8de65a2c931f3308b75233dc039a3fbc3189ab9f0bc25969bdca53b7f01fd
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 13
  VoteEscrow.delegate confirmed   Block: 15246521   Gas used: 153803 (1.28%)

<Transaction '0x1dc8de65a2c931f3308b75233dc039a3fbc3189ab9f0bc25969bdca53b7f01fd'>
>>> x.delegate(1, 1, {"from": a[0]})
Transaction sent: 0xbc4a1654db09ed63698f7be1737dde9c1d43a4ebbcf85d5b5b011ff0b2be06ce
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 14
  VoteEscrow.delegate confirmed   Block: 15246522   Gas used: 175579 (1.46%)

<Transaction '0xbc4a1654db09ed63698f7be1737dde9c1d43a4ebbcf85d5b5b011ff0b2be06ce'>
>>> x.getVotes(1) ## Voting Power of 1 is tripled
37101645733799250 

## Because 1 has delegated to themselves 3 times as duplicated delegates are pushed to the list
>>> x._getCurrentDelegated(1)
(1, 1, 1)

Mitigation Steps

I believe that remediation here won't be straightforward and may require plenty of time to avoid additional vulnerabilities. (see other submissions I've sent, unfortunately quite a few)

That said, mitigation to this exact duplication may be achieved by using a Set or simply by checking for duplicates.

Set Solution

To avoid duplicates you may want to use Openzeppelin's EnumerableSet

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/EnumerableSet.sol

Which can be used in this way

contract DemoSetContract {
    // Example setup
    using EnumerableSet for EnumerableSet.UintSet;

    mapping(uint256 => EnumerableSet.UintSet) public delegates;

    // Example usage
    delegates.add(Id)
    delegates.remove(Id)
}

Check for duplicates solution

You may find a duplicate by looping, then use removeElement to remove the entry from the list

Lines of code

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

Vulnerability details

Because VoteEscrowDelegation.transferFrom is performing an external call to removeDelegation, the msg.sender will always be address(this), causing the function to always revert.

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

        this.removeDelegation(_tokenId);

A simple fix would be to make removeDelegation public and then changing the code to JUMP to it by removing this

POC

Any call will always revert

<Transaction '0xa7b81478677907a369cea2b73fd410ebf7842d46631689cc66e2ba6a1857736b'>
>>> history[-1].call_trace(True)
Call trace for '0xa7b81478677907a369cea2b73fd410ebf7842d46631689cc66e2ba6a1857736b':
Initial call cost  [21940 gas]
VoteEscrow.transferFrom  0:386  [527 / 5285 gas]
└── VoteEscrowCore.transferFrom  146:386  [26 / 4758 gas]
    └── VoteEscrow._transferFrom  154:386  [2659 / 4732 gas]
        │   
        └── VoteEscrow.removeDelegation  [CALL]  229:374  [2073 gas]
                ├── address: 0x6b4BDe1086912A6Cb24ce3dB43b3466e6c72AFd3
                ├── value: 0
                ├── input arguments:
                │   └── tokenId: 1
                └── revert reason: VEDelegation: Not allowed

Lines of code

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

Vulnerability details

NOTE: This finding requires fixing another bug in the codebase:

• Fix underflow on first checkpoint, see: "delegate and removeDelegation are broken due to _writeCheckpoint underflow"

removeDelegation will receive the tokenId of the token to remove the delegation, and it will then remove itself from it's own list of tokens delegated.

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

    /// @notice Remove delegation
    /// @param tokenId TokenId of which delegation needs to be removed
    function removeDelegation(uint256 tokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        uint256 nCheckpoints = numCheckpoints[tokenId];
        Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];
        removeElement(checkpoint.delegatedTokenIds, tokenId);
        _writeCheckpoint(tokenId, nCheckpoints, checkpoint.delegatedTokenIds);
    }

This is fine to remove voting from one-self (if that makes sense)

However this process will not remove the token from any other checkpoint, meaning that any user can create a token of id tokenId and then delegate it to any number of additional tokens tokenA, tokenB, etc.. and all of those will receive delegation, as there seems to be no way to remove the delegation given.

This results in the ability to re-use a token votes by delegating to a myriad of other tokens

POC

## Create a new Lock
>> x.create_lock(1e18, chain.time() / 1000, {"from": a[0]})
Transaction sent: 0x8c6b7a8eb6ed92ab4dbfa27c391981da8b8e8c00c583f0a87bc50e55fa625727
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 15
  VoteEscrow.create_lock confirmed   Block: 15246523   Gas used: 319653 (2.66%)

<Transaction '0x8c6b7a8eb6ed92ab4dbfa27c391981da8b8e8c00c583f0a87bc50e55fa625727'>

## Id is 3
>>> history[-1].return_value
3

## Create a new lock (id will be 4), which we'll delegate to demonstrate the bug
>>> x.create_lock(1,  chain.time() / 1000, {"from": a[1]})
Transaction sent: 0x9bb3b3f1ae6ae69d2631378cff1ebc2f9022644138a665fc0416d8d101c80e0a
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 6
  VoteEscrow.create_lock confirmed   Block: 15246524   Gas used: 296193 (2.47%)

<Transaction '0x9bb3b3f1ae6ae69d2631378cff1ebc2f9022644138a665fc0416d8d101c80e0a'>

## Delegate to 4
>>> x.delegate(3, 4, {"from": a[0]})
Transaction sent: 0x63be9d38cc19443024c6b138bdb85489f7214d957cd46886f98ac9f51c82861c
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 16
  VoteEscrow.delegate confirmed   Block: 15246525   Gas used: 134655 (1.12%)

<Transaction '0x63be9d38cc19443024c6b138bdb85489f7214d957cd46886f98ac9f51c82861c'>

## Prove we have delegated as expected
>>> x._getCurrentDelegated(3)
()

## We delegated to 4 as intended
>>> x._getCurrentDelegated(4)
(3)

## Remove Delegation
>>> x.removeDelegation(3, {"from": a[0]})
Transaction sent: 0xfc7ba1a1b194eeba4f24282a623b7932ecc14606b0f3ed73597d42262eea6d4f
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 17
  VoteEscrow.removeDelegation confirmed   Block: 15246526   Gas used: 66642 (0.56%)

<Transaction '0xfc7ba1a1b194eeba4f24282a623b7932ecc14606b0f3ed73597d42262eea6d4f'>

## It doesn't work, our votes are duplicated for 4 now
>>> x._getCurrentDelegated(4)
(3)

Mitigation steps

An INCOMPLETE remediation would be to check the list of tokens that have been delegated to via

uint[] listToRemove = _getCurrentDelegated(tokenId)

And then looping over it to remove the token from each of those checkpoints

uint[] listToRemove = _getCurrentDelegated(tokenId)

uint length = listToRemove.length
for (uint i; i < length; ) {
    uint256 nCheckpoints = numCheckpoints[tokenId];
    // Note: we also do check for 0 just in case
    Checkpoint storage checkpoint = checkpoints[listToRemove[i]][nCheckpoints > 0 nCheckpoints ? 0];
    
    // Remove our token of `tokenId` from the checkpoint of the token to remove `listToRemove[i]`
    removeElement(checkpoint.delegatedTokenIds, tokenId);

    unchecked { ++i; }
}

Additional Resources

In continuation of the POC you can see how, if we try to remove delegation via 4, we still cannot do that.

Ultimately removeDelegation in it's current implementation only removes the tokenId from it's own checkpoint, which is not the intended functionality

>>> x.removeDelegation(4, {"from": a[1]})
Transaction sent: 0x4895973bc32687d8e26fbbb35eb81832cb43f65a879a603e4e69f1e41fa49a04
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 7
  VoteEscrow.removeDelegation confirmed   Block: 15246528   Gas used: 99995 (0.83%)

<Transaction '0x4895973bc32687d8e26fbbb35eb81832cb43f65a879a603e4e69f1e41fa49a04'>
>>> x._getCurrentDelegated(4)
(3)

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

L05 - Usage of trasfer over call to send Ether could cause unexpected Reverts https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L154-L155

        payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress

The function payEther sends ether via transfer which passes a 2.3k gas stipend to the recipient, this sometimes could not be enough and could cause a Out Of Gas Revert which would prevent users from using the contract.

From my experience transfer is sufficient to send even to a Gnosis Safe, however the contract could break when interacting with other smart contracts

Lines of code

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

Vulnerability details

The code for fillCriteriaBid and fillBid is using transferFrom instead of safeTransferFrom

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

            /// @notice Potentailly Unsafe
            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);

            /// @notice Safe
            nftcontract.safeTransferFrom(msg.sender, o.signer, o.tokenId, amount, '');

Per EIP-721 all ERC721 will have the function safeTransferFrom which ensures the receiver can handle the token they are receiving.

Because the standard guarantees the function to be implemented, considering you are already using safeTransferFrom for ERC1155s, and because safeTransferFrom performs additional checks for safety to the receiver, it is best to simply change the code to use safeTransferFrom

Mitigation Step

Change

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

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

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1006-L1007

Vulnerability details

Because VoteEscrowCore.withdraw calls _burn the function will eventually try to remove the token from msg.sender while it belongs to owner causing a revert

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1006-L1007

    function withdraw(uint256 _tokenId) external nonreentrant {
        assert(_isApprovedOrOwner(msg.sender, _tokenId));

Mitigation Step

Rewrite _burn to:

_removeTokenFrom(owner, _tokenId);

Lines of code

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

Vulnerability details

NOTE: This finding requires fixing another bug in the codebase, see: "TransferFrom doesn't work because of the external call"

After fixing that bug through a simple remediation, another bug will show in the code, which will break composability of the smart contract.

TL;DR

Due to changes to _transferFrom the function will only work for transfers but not when a contract or account uses transferFrom meaning that composability is completely negated.

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

    function _transferFrom(
        address _from,
        address _to,
        uint256 _tokenId,
        address _sender
    ) internal override {
        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

        // remove the delegation 
        // Already fixed per "TransferFrom doesn't work because of the external call"
        removeDelegation(_tokenId);

        // Check requirements
        require(_isApprovedOrOwner(_sender, _tokenId));

_transferFrom rightfully checks for _isApprovedOrOwner to perform a transfer, which enables Pull Payments.

However, the overriden version calls removeDelegation

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

    function removeDelegation(uint256 tokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

removeDelegation will check that the caller is the owner of the token, meaning that transferFrom will always revert if performed by a contract or a approved account.

Remediation Step

Ensure that delegation accounts approval, instead of ownership exclusively.

Change

require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

To

  require(_isApprovedOrOwner(_sender, _tokenId));

Executive Summary

The codebase leverages a couple ideas very well to minimize smart contracts while allowing for a:

• Marketplace for NFTs for both Ask and Bids
• Liquidity Mining Program combining both Actions and Locking through VoteEscrow

After a thorough review, I would recommend more time is invested in polishing up the Smart Contracts, most importantly the newer contracts such as VoteEscrowDelegation.

The code would benefit by spending additional time in adding refactorings and further polish both for saving gas and to make the code more maintainable.

At least 30% of the gas cost can be trimmed down fairly easily and the code could benefit by simplifying it further

See below for actionable advice on how to simplify the code and check the Gas Report for how to squeeze more bang for your buck

Legend:

• U -> Impact is unclear and potentially higher than Low Severity
• L -> Low Severity -> Could cause issues however impact is limited
• R -> Refactoring -> Suggested Code Change to improve readability and maintainability or to offer better User Experience
• NC -> Non-Critical / Informational -> No risk of loss, pertains to events or has no impact

U01 - Incorrect Comment - Reward Emissions will happen on a 24hr cadence, but can happen any time

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L105-L106

        // if 24 hours have passed since last epoch change
        if (block.timestamp > startTime + (epoch) * secsInDay) {

Comment says that 24 hours must have passed, however the math for the check will check if the time since the startTime % secsInDay would bring us to a new epoch, meaning that the change of the epoch can be enacted every day at the same time, however the interval of time between one epoch and another will not necessarily be constant.

In the worst case a epoch could be skipped, or a epoch could last 1 second (or 1 block which would be 13 seconds)

A epoch could last just one block if there has been a day without trades, and at the 23rd hour and 59 minutes, one trade happens.

Epoch would change to Epoch + 1

A minute after another trade could happen, and epochs would change again to Epoch + 2

Recommended Mitigation

Either enforce a 24hr delay between each epoch, or update the comment to inform about the actual behaviour

U02 - Spam Delegation of tokens can be used to deny delegation of valid tokens - Breaking Composability

VoteEscrowDelegation._writeCheckpoint enforces a 500 token limit for delegation, unfortunately this can be used by a malicious attacker to spam with no value tokens.

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

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

The limit could be used to negate derivative, "liquid lockers" (See bveOXD by BadgerDAO) or delegation based autocompounders. (See Union for vlCVX)

Mitigation Steps

An alternative system, that sums the total voting power of a tokenId, and uses storage mapping for delegation may prove resilient against DOS (due to not looping)

U03 - Admin Privilege / Rug Risk - Owner can mint by changing minter

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L58

function setMinter(address _minter) external onlyOwner {

Because of the fact that minter can be changed, the owner has the ability to mint as many tokens as they'd like.

A mitigation step of marking setMinter take an extra day has been taken.

However it would be best to lock in the minter as an immutable variable to ensure only RewardDistributor can ever mint new tokens, giving stronger security guarantees to end users (and making operations cheaper)

U04 - Admin Privilege / Rug Risk - Owner can rug rewards by changing trader

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L285-L288

function changeTrader(address _trader) external onlyOwner {

Similarly to setMinter changeTrader allows the owner to change the contract that is reporting trades, which indirectly has control over tokens minted, meaning those tokens could be accrued to an arbitrary address.

A mitigation step of marking changeTrader take an extra day has been taken.

However it would be ideal to set the trader as immutable to prevent reward math from being changeable in an unfair way

L01 - Variable Shadowing in VoteEscrowDelegation

While no funds are at risk, variable shadowing is a strong code-smell as a distracted developer may end up confusing a variable from storage with a function parameter

The following 2 variables are repeatedly declared as local variables but their names clash with storage variables:

• tokenId
• checkpoint

Mitigation

Consider changing the names to the convention you've already used in VoteEscrowCore

e.g. Change

    /// @notice Remove delegation
    /// @param tokenId TokenId of which delegation needs to be removed
    function removeDelegation(uint256 tokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        uint256 nCheckpoints = numCheckpoints[tokenId];
        Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];
        removeElement(checkpoint.delegatedTokenIds, tokenId);
        _writeCheckpoint(tokenId, nCheckpoints, checkpoint.delegatedTokenIds);
    }

To

    /// @notice Remove delegation
    /// @param tokenId TokenId of which delegation needs to be removed
    function removeDelegation(uint256 _tokenId) external {
        require(ownerOf(_tokenId) == msg.sender, 'VEDelegation: Not allowed');
        uint256 nCheckpoints = numCheckpoints[_tokenId];
        Checkpoint storage _checkpoint = checkpoints[_tokenId][nCheckpoints - 1];
        removeElement(_checkpoint.delegatedTokenIds, tokenId);
        _writeCheckpoint(_tokenId, nCheckpoints, _checkpoint.delegatedTokenIds);
    }

Additional Instances

• delegate
• _getCurrentDelegated
• getVotes
• getPriorVotes
• removeDelegation

L02 - Inexact Implementation of safeTransferFrom

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L604-L605

safeTransferFrom is supposed to check that the receiving contract not only implements the function onERC721Received but also that the returned value is exactly bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")).`

Mitigation Step:

Implement the exact check per the Spec: https://eips.ethereum.org/EIPS/eip-721

L03 - Dust in AddFee will cause minor dust losses

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L119-L121

            rewardStaker[epoch] = stakerReward;
            rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
            rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;

Due to integer division, minor amounts of dust will be lost and not tracked.

It would be better to perform the divisions and lastly use subtraction to ensure that the remaining amount captures that dust.

There's a 50% change of getting 1 wei of dust on each division (odd number), from my testing there's 98% chance of getting 1 wei on each function call.

See Python Script:

from random import seed
from random import random

NUMBER_OF_SIMS = 10_000
DAILY_TOKEN_MAX = 600000

"""
  rewardStaker[epoch] = stakerReward;
  rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
  rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;
"""

def simulation():
  tokenToEmit = int(random() * DAILY_TOKEN_MAX)
  stakerReward_percent = int(random() * 100)
  stakerReward = tokenToEmit * stakerReward_percent // 100
  rewardTrader = ((tokenToEmit - stakerReward) * 67) // 100
  rewardExchange = ((tokenToEmit - stakerReward) * 33) // 100

  total = stakerReward + rewardTrader + rewardExchange
  dust = tokenToEmit - total

  return dust

def main():
  counter = 0
  for x in range(NUMBER_OF_SIMS):
    dust = simulation()
    if dust != 0:
      counter += 1
  
  print("counter")
  print(counter)

Running it

Running 'scripts/dust_golom_sim.py::main'...
counter
9880

Mitigation Step

Change the code to do a subtraction from the tokens left

rewardExchange = tokenToEmit - stakerReward - rewardTrader

This will save the cost of a divison (5 gas for DIV vs 3 gas for SUB), and will avoid the rounding error

L04 - You can delegate to a non-existing NFT

Due to a lack of checks delegate can be called on an tokenId that doesn't exist

https://github.com/GalloDaSballo/brownie-starter/blob/28f708ebd8cfb4fa1a412df9fa376d33813f281f/contracts/VoteEscrowDelegation.sol#L67-L68

    function delegate(uint256 tokenId, uint256 toTokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

Proof of Concept

>>> x = VoteEscrow.deploy(ETH_ADDRESS, {"from": a[0]})

>>> x.create_lock(123, chain.time() / 1000, {"from": a[0]})

## I'm able to delegate to a non existing tokenId
>>> x.delegate(1, 12, {"from": a[0]})
Transaction sent: 0x3b76a56fc70dea99aa07226b20511b380b7cb767118cf251244559a9c8d99bb5
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 8
  VoteEscrow.delegate confirmed   Block: 15246417   Gas used: 134655 (1.12%)

Mitigation Step:

A simple check for existence would prevent that from happening

require(ownerOf(toTokenId) != address(0), 'VEDelegation: !Token');

L05 - Usage of trasfer over call to send Ether could cause unexpected Reverts

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L154-L155

            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress

The function payEther sends ether via transfer which passes a 2.3k gas stipend to the recipient, this sometimes could not be enough and could cause a Out Of Gas Revert which would prevent users from using the contract.

From my experience transfer is sufficient to send even to a Gnosis Safe, however the contract could break when interacting with other smart contracts

R01 - Change code to require exact amount

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

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

GolomTrader.fillAsk is checking that the msg.value is sufficient to cover all costs, however the check allows for more ETH than necessary to be sent.

Additionally the >= check costs more gas.

I'd recommend refactoring the code to require an exact amount, ensuring no dust amounts of ETH are sent to the contract needlessly

R02 - Usage of Magic Values

Consider instead using constants, this will help make the code easier to maintain

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L212-L213

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

R03 - Inconsistent naming for public function

_verifyProof is marked as public, however it's name is prefixed by an _ which is used throghout the code to signify internal / private functions

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L409-L410

    function _verifyProof(
        uint256 leaf,
        bytes32 root,
        bytes32[] memory proof
    ) public pure {

Recommended Mitigation

Change to

    function _verifyProof(
        uint256 leaf,
        bytes32 root,
        bytes32[] memory proof
    ) internal pure {

Or rename the function to verifyProof

NC01 - You can spam create new checkpoints via removeDelegation

removeDelegation is meant to reset the delegation for a given tokenId

However the function can be called repeteadly, to no effect beside increasing the amount of checkpoints numCheckpoints

This will increase cost of operation but has no major impact.

Mitigation Step:

If no delegation was given, revert instead of adding more entries to checkpoint[tokenId]

NC02 - Valid orders of any kind can be created as long as o.signer is set to address(0)

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/HighSeverityGolomTrader.sol#L23-L24

        require(signaturesigner == o.signer, 'invalid signature');

Because ecrecover will return address(0) on failure, any order can be constructed by setting the o.signer as address(0).

Remediation

Consider reverting when signaturesigner is address(0)

signaturesigner != address(0)

NC03 - No Direct Support for CryptoPunks

Because Cryptopunks are not conformant to ERC721, the system will not allow trading them.

While there's no vulnerability here, this may be a wasted opportunity.

An alternative that alleviates this is to use WrappedPunks https://www.wrappedpunks.com/

Mitigation Step

Document how users can still trade CryptoPunks by wrapping them

NC04 - You can create Orders to Yourself

Because of a lack of checks for o.signer and receiver, orders can be placed by the same account.

This can be used to produce orders that have no cost beside the fees, as a way to game the liquidity mining program.

Beside gaming fees, which can have negative price impact on the GolomToken, I don't believe this to be cause for any vulnerabilities

NC05 - You can create 0 Value Orders

Because of the lack of checks for o.totalAmt, anyone can create orders with zero cost.

This has no practical usage beside the cost of gas and the emission of the event which may allow to display spam orders on a UI

Mitigation Steps

Consider whether it's ok to have 0 value orders, and if you'd want to hide them from your UIs

NC06 - Lack of events in Set / Accept functions

Most setters don't have an event, making off-chain tracking (and TheGraph) integrations more difficult

setMinter executeSetMinter https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L56-L72

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L442-L457 setDistributor executeSetDistributor

Also for one off minting mintGenesisReward mintAirdrop

NC07 - Code / Comment Discrepancy

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L396-L397

Throws if _owner is the zero address. NFTs assigned to the zero address are considered invalid.

Function will return 0 instead of throwing with zero-address

Interestingly the code from CRV will also not throw: https://etherscan.io/address/0x5f3b5dfeb7b28cdbd7faba78963ee202a494e2a2#code#L526

Mitigation Step:

Either delete the comment, or add a require for address(0)

NC08 - Rolled your own Merkle Validation - Safe

Thought I'd mention that I did check your Merkle Validation and while some variable names have been changed, it ultimately is consistent with how OZ does it.

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L405-L428

NC09 - Minor dust on reward distribution and Golom Trader

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L273-L276

            reward =
                reward +
                (rewardExchange[index] * feesExchange[addr][index]) /
                epochTotalFee[index];

Due to integer division as well as the constant changes between epochTotalFee and rewardExchange, minor amounts of dust will accrue in the contract.

It may be worth having a function to sweep the dust, perhaps after all token distribution was zero for a certain amount of days.

My estimate is that there's a 50% chance (even / odd) that 1 wei of dust may be left in the contract, for each division that is done.

Meaning 1 million claims would be needed for 1e6 tokens worth of dust to be left in the contract.

For that reason, it's not economically worth it to track those amounts, but it may still be worth it to sweep it eventually

Also:

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L258-L261

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L229-L244

Also see GolomTrader:

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L250-L270

NC10 - Notice on Unchecked ERC20 Returns

I have no doubt other warden will submit the fact that the code is using transfer without a checked return.

rewardToken.transfer(addr, reward)

However I believe that the usage is fine as we know that rewardToken is GolomToken which is ERC20Votes which inherits from OZs ERC20 which as you can see will revert on error: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ce0068c21ecd97c6ec8fb0db08570f4b43029dde/contracts/token/ERC20/ERC20.sol#L226

NC11 - You can claim tokens on someone elses behalf

While no vulnerability was found, the functions:

• traderClaim
• exchangeClaim
• multiStakerClaim

Allow to perform a claim on someone elses behalf.

This can cause issues for the following two cases:

• Triggers Taxable Gains
• Smart Contracts may be expecting to calculate a delta of tokens, someone else claiming may throw the math off

While not a vulnerability, this can cause issues for integrators or gotchas for developers.

We had a similar gotcha at Badger when integrating with Aura Finance: https://github.com/Badger-Finance/vested-aura/commit/475119a678dbc679781ce95edb3bfaff0bf9175f#diff-f8da72ae916d33dc152fcb1f98526aee546d93502123f04280e848bc82e20532R236#L236

NC12 - Typos / Grammar

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L140-L141

    // allows sellers of nft to claim there previous epoch rewards

Recommendation: allows sellers of NFT to claim their previous epoch rewards

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L154-L155

    // allows exchange that facilated the nft trades to claim there previous epoch rewards

Recommendation: Allows the exchange that facilitated the NFT trades to claim their previous epoch rewards

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L168-L169

    /// @dev allows VeNFT holders to claim there token and eth rewards

Recommendation: allows VeNFT holders to claim their token and eth rewards

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

/// @dev returns unclaimed golom rewards of a trader

Recommendation: Returns unclaimed Golom rewards for a trader

Executive Summary

Most contracts can benefit by receiving further gas optimizations.

I roughly estimate that at least 30% of gas cost can be deducted by applying the suggestions below.

All suggestions are sorted by impact and ease of application.

At the bottom I list a set of "usual suspects" which I assume most other wardens will have sent.

Unless otherwise noted, all gas savings are estimates, where benchmarks are provided they are generated via Gas Lab

Huge gas savings with immutable - Up to 6k in addFee (each settlement), 2.1k+ for other functions

Variables that never change would be better of set as immutable.

An immutable variable doesn't occupy storage, saving thousands of gas.

This will save a 2.1k gas cost each time they are read the first time and an additional 100 gas for each subsequent read.

Because addFee is called when orders are settled I highly recommend applying this fix as it will make it at least 6.1k gas cheaper to settle an order.

Make the following variables immutable:

• RewardDistributor.weth -> 2.1k from claim, 2.1k from addFee
• RewardDistributor.rewardToken -> 2.8k from addFee, 2.1k everywhere else
• RewardDistributor.startTime -> 2.1k from addFee

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L300-L301

• VoteEscrowCore.token

Will save 2.1k for each time a transfer happens (create_lock, increase_amount)

Lack of early returns will cost up to 6.3k extra gas - Up to 6.3k gas

The check _isApprovedOrOwner is meant to help verify if allowance was given.

Because allowance is calculated as a Or statement, an early return on the first true value would avoid additional reads from Storage.

In the case of the owner performing a transfer, the early return will save up to 6.3k gas (3 less SLOADs from cold slots)

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L433-L443

    /// @dev Returns whether the given spender can transfer a given token ID
    /// @param _spender address of the spender to query
    /// @param _tokenId uint ID of the token to be transferred
    /// @return bool whether the msg.sender is approved for the given token ID, is an operator of the owner, or is the owner of the token
    function _isApprovedOrOwner(address _spender, uint256 _tokenId) internal view returns (bool) {
        address owner = idToOwner[_tokenId];
        bool spenderIsOwner = owner == _spender;
        if (spenderIsOwner) {
            return true;
        }

        bool spenderIsApproved = _spender == idToApprovals[_tokenId];
        if (spenderIsApproved) {
            return true;
        }

        bool spenderIsApprovedForAll = (ownerToOperators[owner])[_spender];
        if  (spenderIsApprovedForAll) {
            return true
        }

        // This is what peak performance looks like
        
        return false
    }

This can be further optimized to perform the comparisons inside the ifs, however the refactoring showed above will saved 3 orders of magnitude more gas and as such I highly recommend it

Packing and shrinking of variable to save more gas - 20k gas saved on minter change - 15k gas saved on isGenesisRewardMinted and isAirdropMinted - Up to 35k gas saved

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L17-L18

    uint256 public minterEnableDate;

    address public pendingMinter; // Move pendingMinter above so it goes in slot2
    uint64 public minterEnableDate; // Change to uint64 it will pack with the above
    

    bool public isAirdropMinted; // These will also pack with the above
    bool public isGenesisRewardMinted; // These will also pack with the above

Resorting Variable Declaration and then setting minterEnableDate to uint64 will cause it to be packed with

• pendingMinter
• isAirdropMinted
• isGenesisRewardMinted

uint64 represents up to 1.8446744e+19 meaning it will work until the year 2286

Because of this new packing, when setting a new pendingMinter both the address and the timestamp will be in the same slot.

When changing pendingMinter from zero to non-zero, that's going to save 20k gas (cost of setting value from zero to non-zero) as the slot will remain non-zero

For similar reasons isAirdropMinted and isGenesisRewardMinted will also keep the slot non-zero, triggering further gas savings of up to 15k gas (non-zero to non-zero SLOAD costs 5k gas)

Replace 0 and 1 with 1 and 2 to avoid triggering gas refunds - Up to 15k gas per operation

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L883-L894

    function attach(uint256 _tokenId) external {
        require(msg.sender == voter);
        attachments[_tokenId] = attachments[_tokenId] + 1;
    }

    function detach(uint256 _tokenId) external {
        require(msg.sender == voter);
        attachments[_tokenId] = attachments[_tokenId] - 1;
    }

    function merge(uint256 _from, uint256 _to) external {
        require(attachments[_from] == 0 && !voted[_from], 'attached');

By using 0 as the null value instead of 1, you're triggering gas refunds and then extra costs As any time the value goes from 0 to non-zero, that's 20k gas vs 5k for a non-zero to non-zero (would only happen once)

Checking for a lack of attachment could be changed to:

uint256 cachedAttachment = attachments[_from];
require(cachedAttachment == 0 || cachedAttachment == 1)

Which can be further optimized (perhaps a < 2), however the bulk of the savings are in not triggering refunds

Reduce SLOADs in addFee - 1200 gas

Beside the advised immutable variables, epoch which is a storage variable is read 14 times in addFee

Using a memory cached version of oldEpoch = epoch as well as a new epoch after the increment, will reduce the number of storage reads, removing upwards of 12 * 100 = 1200 gas

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

Double Storage Read - Save by Caching 94 GAS

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

Don't re read storage vars - 97 GAS per trade

filled[hashStruct] is used in validateOrder to calculate amountRemaining

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L224-L229

        (uint256 status, bytes32 hashStruct, uint256 amountRemaining) = validateOrder(o);

        require(status == 3, 'order not valid');
        require(amountRemaining >= amount, 'order already filled');

        filled[hashStruct] = filled[hashStruct] + amount;

To save gas later, it may be best to return it from validate order (filledAmount)

This will allow to save 100 gas later (at the cost of 3 gas), by changing:

filled[hashStruct] = filled[hashStruct] + amount;

To

filled[hashStruct] = filledAmount + amount;

Validation function can be rewritten to pass the value, saves 100 gas per trade

Massive gas optimizations for loops, don't compare iterator against storage value - 97 per iteration

Because epoch is from storage, each iteration of the loop will costs 100 extra gas to re-load epoch

Using a supporting memory variable will save 94 gas the first time and 97 time each subsequent iteration

Refactor: for (uint256 index = 0; index < epoch; index++) {

To: uint256 cachedEpoch = epoch; for (uint256 index = 0; index < cachedEpoch; index++) {

Also see further below for additional minor gas savings (25 / 30 per iteration of loops)

Instances:

• stakerRewards
• `traderRewards
• `exchangeRewards

Corollary to the above - 97 per iteration

Additional functions also check epoch against the provided parameter, because you'd expect the provided epochs array to have a length above 1, you'd be better of caching the value of epoch.

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L156-L157

Change

        for (uint256 index = 0; index < epochs.length; index++) {
            require(epochs[index] < epoch);

To

        uint256 cachedEpoch = epoch
        for (uint256 index = 0; index < epochs.length; index++) {
            require(epochs[index] < cachedEpoch);

Instances:

• exchangeClaim
• traderClaim
• multiStakerClaim

Usual Gas Optimizations

Below are listed minor gas optimizations that can help make heavily used functions cheaper, consider defaulting to these any time you're writing new code as there's no "drawback" just gas savings

Optimize Loops (use unchecked { ++i }), also avoid default assignment - Saves 25 + 3 + 3 gas per instance

Each iteration of the loop will save the following

Saves 5 gas on going from i++ to ++i

Saves 20 gas via unchecked

Saves 3 gas in skipping the i = 0 default assignment (declaring uint256 i; is sufficient)

Save another 3 gas per iteration by storing the length of the array in a memory variable (finding length of an array requires 6 gas as you're checking the offset of memory and then storing that value in memory)

uint256 length = proof.length;

This will save even more gas if the values are from storage, however the Storage savings are reported above as they save more gas

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L415-L416

        for (uint256 i = 0; i < proof.length; i++) {

Grep output below for you to mass replace

rewards/RewardDistributor.sol

142:        for (uint256 index = 0; index < epochs.length; index++) {
156:        for (uint256 index = 0; index < epochs.length; index++) {
182:            for (uint256 index = 0; index < epochs.length; index++) {
225:        for (uint256 index = 0; index < epoch; index++) {
257:        for (uint256 index = 0; index < epoch; index++) {
272:        for (uint256 index = 0; index < epoch; index++) {

rewards/RewardDistributor.sol

179:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {

vote-escrow/VoteEscrowDelegation.sol

171:        for (uint256 index = 0; index < delegated.length; index++) {
189:        for (uint256 index = 0; index < delegatednft.length; index++) {

vote-escrow/VoteEscrowDelegation.sol

199:        for (uint256 i; i < _array.length; i++) {

For these just make the increment unchecked - For these it will save 20 gas with the unchecked + 3 by skipping the assignment

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) {

Change function to external and most importantly data to calldata - 1028 gas saved

Memory means the data will be copied from calldata, and copied to memory

You can skip the overhead by declaring the function external and the data location to calldata

See benchmark below to prove that this will save around 1k gas per claim

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

    function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {

    
    // Change to
    function multiStakerClaim(uint256[] calldata tokenids, uint256[] calldata epochs) external {

Instances:

• addFee (will save at least 6 gas here)
• traderClaim
• exchangeClaim
• multiStakerClaim

Code

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.13;

import "../../lib/test.sol";
import "../../lib/Console.sol";

contract GasTest is DSTest {
    CalldataExternalTest c0;
    MemoryPublicTest c1;

    function setUp() public {
        c0 = new CalldataExternalTest();
        c1 = new MemoryPublicTest();
    }

    function testGas() public {
        uint256[] memory tokenids = new uint256[](5);
        uint256[] memory epochs = new uint256[](5);
        c0.multiStakerClaim(tokenids, epochs);
        c1.multiStakerClaim(tokenids, epochs);
    }
}

contract CalldataExternalTest {
    function multiStakerClaim(uint256[] calldata tokenids, uint256[] calldata epochs) external {
        // Do nothing
    }
}

contract MemoryPublicTest {
    function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {
        // Do nothing
    }
}

Output

Test result: ok. 1 passed; 0 failed; finished in 3.34ms
╭───────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ CalldataExternalTest contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost               ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 58511                         ┆ 324             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                 ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ multiStakerClaim              ┆ 661             ┆ 661 ┆ 661    ┆ 661 ┆ 1       │
╰───────────────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯
╭───────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ MemoryPublicTest contract ┆                 ┆      ┆        ┆      ┆         │
╞═══════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost           ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 79929                     ┆ 431             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name             ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ multiStakerClaim          ┆ 1689            ┆ 1689 ┆ 1689   ┆ 1689 ┆ 1       │
╰───────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯

Corollary to the above - Changes to VerifyProof - 553 gas saved

Because the data passed to _verifyProof is calldata you can change the location for it to calldata as well, while keeping it internal to save gas that would be used for the copy of data from calldata to memory

See:

• _verifyProof

Benchmark for _verifyProof changes

Costly -> public memory

│ Function Name        ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls

│ verifyProof          ┆ 2634            ┆ 2634 ┆ 2634   ┆ 2634 ┆ 1     

Fixed -> external calldata

│ Function Name       ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls

│ verifyProof         ┆ 2081            ┆ 2081 ┆ 2081   ┆ 2081 ┆ 1

Make toString fully unchecked - 556 gas

The function toString is based on code written in Solidity 0.4, which didn't use checked math

You can save around 550 gas by simply adding an unchecked block around the whole code

Original Cost:

Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls toString ┆ 2434 ┆ 2434 ┆ 2434 ┆ 2434 ┆ 1

Rewrite to

function toString(uint256 value) external pure returns (string memory) {
    // Inspired by OraclizeAPI's implementation - MIT license
    // https://github.com/oraclize/ethereum-api/blob/b42146b063c7d6ee1358846c198246239e9360e8/oraclizeAPI_0.4.25.sol
    unchecked {
        if (value == 0) {
            return '0';
        }
        uint256 temp = value;
        uint256 digits;
        while (temp != 0) {
            digits++;
            temp /= 10;
        }
        bytes memory buffer = new bytes(digits);
        while (value != 0) {
            digits -= 1;
            buffer[digits] = bytes1(uint8(48 + uint256(value % 10)));
            value /= 10;
        }
        return string(buffer);
    }
    }

Updated Gas Cost: (Tested over 50k iterations)

│ Function Name       ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls

│ toString            ┆ 602             ┆ 1827 ┆ 1878   ┆ 1878 ┆ 50000