Golom contest - kenzo's results

Lines of code

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

Vulnerability details

When writing the first checkpoint, _writeCheckpoint tries to get the previous checkpoint, but that leads to an underflow and revert.

Impact

No delegations can be made.

Proof of Concept

When a user delegates to a token for the first time, nCheckpoints would be 0:

        uint256 nCheckpoints = numCheckpoints[toTokenId];

This 0 amount is then sent to _writeCheckpoint:

            _writeCheckpoint(toTokenId, nCheckpoints, array);

_writeCheckpoint then tries to get the previous checkpoint by substracting 1 from nCheckpoints:

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

But as nCheckpoints is 0, this will underflow and revert.

Recommended Mitigation Steps

Assign to oldCheckpoint only if nCheckpoints != 0.

Lines of code

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

Vulnerability details

For checking how much voting power each delegate has, VoteEscrow saves a delegatedTokenIds which contains all the tokens that delegated to the delegate. But when changing delegates, VoteEscrow does not remove the token from the previous delegate's delegatedTokenIds.

Impact

Voting power is never removed after changing delegation. Infinite voting power can be attained by delegating the same token again and again.

Proof of Concept

We can see in the delegate function that it only updates the new delegate, but does not remove the token from the previous delegates[tokenId] (if it existed).

    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);
            _writeCheckpoint(toTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
        } else {
            uint256[] memory array = new uint256[](1);
            array[0] = tokenId;
            _writeCheckpoint(toTokenId, nCheckpoints, array);
        }

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

This also makes it possible for someone to delegate the same token again and again to get infinite voting power.

Recommended Mitigation Steps

If delegates[tokenId] exists, remove tokenId from delegates[tokenId]'s delegatedTokenIds using the same checkpoint mechanism. Also to save some effort consider reverting if delegates[tokenId]==toTokenId.

Lines of code

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

Vulnerability details

When trying to transfer VE NFTs, the contract executes an external call to itself. This changes msg.sender and will cause the token owner check to fail.

Impact

Transfer will revert, VE tokens can not be transferred.

Proof of Concept

VoteEscrow's _transferFrom executes an external call to itself:

        this.removeDelegation(_tokenId);

Such a call (using this.) will change the context and make VoteEscrow be the msg.sender.

removeDelegation checks if the token's owner is msg.sender:

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

But since the VoteEscrow is now the sender, this check will fail, and tokens can not be transferred.

Recommended Mitigation Steps

Change removeDelegation to be public instead of external and call it internally via doing removeDelegation(_tokenId);.

Lines of code

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

Vulnerability details

To update delegation checkpoints, _writeCheckpoint is called. In case there was already a checkpoint added in the current block, the function tries to update that checkpoint - but it updates it in memory, not in storage.

Impact

Changes made to checkpoints on the same block will not be persisted.

Proof of Concept

delegate calls _writeCheckpoint. _writeCheckpoint takes the latest checkpoint, stores it in memory, and tries to update it:

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

        if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
            oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;

But since the assignment is only happening on a memory variable, it won't get persisted to the storage. Therefore in a certain block only the first change to a token's delegated token list will be persisted - all later changes done on the same block will be lost.

Note: before calling _writeCheckpoint , delegate is actually updating the latest checkpoint:

            Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
            checkpoint.delegatedTokenIds.push(tokenId);
            _writeCheckpoint(toTokenId, nCheckpoints, checkpoint.delegatedTokenIds);

But as I detailed in another issue, that is a separate mistake, as it means that whenever a new checkpoint needs to be created, the previous checkpoint will be wrongly updated as well. So there are 2 issues: When updating delegates on the same block, only the first update is persisted - this issue, and when creating a new checkpoint, the previous checkpoint is wrongly updated - previous issue. So although the bug of the current issue in not present in the code right now - as delegate updates the storage variable - that it updates it is a mistake in itself which should be changed. The separate mitigation of these 2 different issues should make the process whole.

Recommended Mitigation Steps

In _writeCheckpoint, change oldCheckpoint's location to be storage instead of memory.

Lines of code

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

Vulnerability details

The contract is accidently editing both the previous and current checkpoint when changing/removing a delegate.

Impact

Incorrect counting of votes.

Proof of Concept

If in delegate the delegate already has checkpoints, the function will grab the latest checkpoint, and add the tokenId to it. Note that it changes the storage variable.

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

It then calls _writeCheckpoint, which will add a new checkpoint if there's no checkpoint created for this block yet:

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

Therefore, if this function has created a new checkpoint with the passed _delegatedTokenIds, we already saw that the previous function has already added tokenId to the previous checkpoint, so now both the new checkpoint and the previous checkpoint will have tokenId in them. This is wrong as it updates an earlier checkpoint with the latest change.

The same situation happens in removeDelegation.

Recommended Mitigation Steps

When reading the latest checkpoint:

Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];

Change the storage to memory. This way it will not affect the previous checkpoint, but will pass the correct updated array to _writeCheckpoint, which will then write/update the correct checkpoint.

Lines of code

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

Vulnerability details

The removeDelegation function tries to remove the supplied token from the delegated tokens list of that token itself, and not of who it delegated to.

Impact

If the supplied token has been delegated to another token, the delegation can not be removed.

Proof of Concept

The function removes the token from it's own list of delegated tokens. Notice the 3rd and 4th lines below. Therefore, if it had been delegated to somebody else, it won't get removed from that list.

    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);
    }

Recommended Mitigation Steps

nCheckpoints and checkpoint should access delegates[tokenId] instead of tokenId.

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

7. transfer is used to transfer Ether Description: GolomTrader uses transfer method to send Ether. This is generally less recommended these days (see article from Consensys Diligence), mainly because gas costs can change in the future and smart contracts (including multisigs such as Gnosis Safes - see article) even now would not be able to receive the Ether. There are other ways to protect against reentrancy, which you've already employed - correct usage of CEI pattern and reentrancy locks. Therefore the usage of transfer is not needed to protect from reentrancy AFAIS. Mitigation: Send Ether using call instead of transfer.

Lines of code

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

Vulnerability details

VE's _burn function (which is called from merge and withdraw) is allowed to be called not only from the token's owner, but also from the owner's operator or whoever was approved for the token. It then tries to remove the token from the sender's tokens, instead of the owner.

Impact

Approval mechanism not working.

Proof of Concept

The _burn function verifies that the sender is the owner or approved:

        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

It emits a log that the token was burned from owner:

        emit Transfer(owner, address(0), _tokenId);

But it tries to remove the token from msg.sender, not from owner.

        _removeTokenFrom(msg.sender, _tokenId);

Therefore, if the sender is not the owner but just the approved, the call will fail as _removeTokenFrom only succeeds if the first parameter sent to it is the owner.

Recommended Mitigation Steps

Remove the token from the owner, not the sender - change the line to _removeTokenFrom(owner, _tokenId);.

Lines of code

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

Vulnerability details

The merge and withdraw functionalities burn a token when called. If the burned token has delegated to somebody, this token is not removed from the delegate's list.

Impact

The delegate's delegated token list will have burned tokens inside it, which can not be removed. This can lead to DOS as the delegated token list is limited to 500. (I also submitted previously the issue that generally speaking, a delegate should be able to remove tokens from his list - however I believe this is a separate issue - this is a core functionality of the VE tokens, and there's no justification for the owner having to routinely call a clean method that will delete legitimate burned tokens from his list.)

Proof of Concept

The burn and withdraw methods are present only in VECore and not in VEDelegation. They do not remove the token from the delegate's list.

Note that _burn is not overwritten in VEDelegation, unlike _transferFrom.

Recommended Mitigation Steps

Similar to how _transferFrom was handled, overwrite _burn in VEDelegation and add removeDelegation(_tokenId);.

Lines of code

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

Vulnerability details

VE's _transferFrom (which all transfer functions call) is overwritten in VEDelegation in order to remove the delegation of the transferred token. This method calls removeDelegation, which reverts if the sender is not the token owner.

Impact

Transfer via approval mechanism won't work; accounts that have been approved to transfer the token can not transfer it.

Proof of Concept

The overwritten _transferFrom checks if the caller is the owner or approved, and calls removeDelegation:

        // remove the delegation
        this.removeDelegation(_tokenId);

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

As this is a general _transferFrom we know it should work with the ERC721 approval mechanism, and also the require above implies that - accounts which have been approved to transfer the token should be able to transfer it. However, it calls removeDelegation(*), and removeDelegation only allows the owner to call it:

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

Therefore, accounts which fulfill _isApprovedOrOwner but are not the owner can not transfer the token.

(*) There's a separate problem here where the call should be internal, not external. I've submitted it in a different issue.

Recommended Mitigation Steps

Divide removeDelegation into 2 functions, internal and external. The external will check that the token owner called it, and then call the internal function. The internal will actually remove the token from the delegated tokens list. And _transferFrom will call the internal function, as it already does by itself the _isApprovedOrOwner check.

You can also choose to let approved accounts change the token's delegation using removeDelegation, therefore just the owner check would need to be changed, but I think that's probably not wanted.

Lines of code

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

Vulnerability details

Each delegate has a maximum of 500 tokens that can delegate to it. A delegate does not have a way to remove tokens that delegated to it.

Impact

DOS is possible. A delegate may get many small votes (legitimate or malicious), and won't be able to remove them. See POC.

Proof of Concept

In one of the latest MakerDAO governance rounds we saw that there were many vote changes just before the proposal closed. Imagine that before a proposal closes, a malicious Malaclypse delegates a lot of minimum-voting-power tokens to legitimate delegate Del. Such that the 500 delegated tokens limit is reached. Now people can not delegate to Del any more. He has been DOSd. Del can delegate his own tokens to another token, but all the people that delegated to him need to do the same.

Recommended Mitigation Steps

Add a function that allows a token owner to remove specific tokens from his delegated tokens list.

I present the following issues: 1. Ineffective RewardDistributor epoch 0 treatment 2. Rewards view functions will revert if one epoch's total fee is 0 3. Ungraceful underflow when trying to fill cancelled order 4. Consider using Enums 5. Unreachable clause in validateOrder 6. GolomTrader can receive accidently-sent Ether 7. transfer is used to transfer Ether 8. RewardDistributor addFee: more meaningful input names 9. Comments issues 10. Some requires have no revert string 11. VoteEscrowCore balanceOfNFTAt does not check ownership change 12. Ungraceful underflow when calling removeDelegation with no checkpoints 13. No timelock for changeMinVotingPower 14. Unused variables in RewardDistributor 15. RewardDistributor - No logs emitted, no convenient way for user to get his unclaimed epochs 16. Insufficient order amounts checks in fill bids functions

1. Ineffective RewardDistributor epoch 0 treatment

Description: When adding fees:

            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}();
                }
            }

The treatment for epoch 0 doesn't seem to work. The comments describe that the first trade will move the epoch to epoch 1. But since no trades have been made, previousEpochFee > 0 will return false, the function won't enter the if clause, and the Ether in the contract will not get deposited into WETH nor will epochTotalFee[0] be updated. Therefore the epoch 0 treatment looks ineffective. Mitigation: move the epoch == 1 check to outside the if (previousEpochFee > 0) clause.

2. Rewards view functions will revert if one epoch's total fee is 0

Description: The view functions traderRewards and exchangeRewards start from epoch 0, run on all epochs, and each epoch divide a calculation by epochTotalFee[index]. If one of these is 0, the functions will revert. As can be seen from the previous point, at the moment epoch 0's total fee would be 0. (assuming the first trade changes the epoch to epoch 1 as the team described). Therefore the functions will always revert. This issue (possible division by 0) is also present in the actual claiming functions, but there the epochs to be claimed are taken as input so the user can ignore the 0 ones. Mitigation: only divide if epochTotalFee[index] > 0. Consider doing the same in the actual claiming functions to prevent reverts and loss of user funds.

3. Ungraceful underflow when trying to fill cancelled order

Description: When a user cancels an order, cancelOrder sets filled[hashStruct] = o.tokenAmt + 1. Later if another user will try to fill this order, validateOrder will be called, and will return o.tokenAmt - filled[hashStruct]. But because of the previous assignment, this is -1 and will underflow, causing a revert. Note that the fill functions later check the amount returned and try to revert gracefully if it is already filled: require(amountRemaining >= amount, 'order already filled'). But this won't be reached due to the underflow. Mitigation: Either set filled[hashStruct] = o.tokenAmt in cancelOrder or return 0 if o.tokenAmt < filled[hashStruct] in validateOrder.

4. Consider using Enums

The project sets special integer values for orderStatus and orderType. Although not very complex, using Enums for that will make the code more clear. Reference: orderType, orderStatus.

5. Unreachable clause in validateOrder

Description: validateOrder has the following block of code:

        require(signaturesigner == o.signer, 'invalid signature');
        if (signaturesigner != o.signer) {
            return (0, hashStruct, 0);
        }

The if clause is never reachable. Mitigation: remove the if clause.

6. GolomTrader can receive accidently-sent Ether

Description: The contract has default payable fallback and receive functions. While the contract needs to be able to receive Ether from the saved WETH contract, it does not need to receive it outside of it's functions from general users. Impact: Users might accidently send Ether to the contract. These funds can then be taken by fraudulent orders. (For example by filling a bid with 0 amount and p that amounts to the misplaced funds). Mitigation: remove the fallback function, and only allow receive to be called from the saved WETH address.

7. transfer is used to transfer Ether

Description: GolomTrader uses transfer method to send Ether. This is generally less recommended these days (see article from Consensys Diligence), mainly because gas costs can change in the future and smart contracts (including multisigs such as Gnosis Safes - see article) even now would not be able to receive the Ether. There are other ways to protect against reentrancy, which you've already employed - correct usage of CEI pattern and reentrancy locks. Therefore the usage of transfer is not needed to protect from reentrancy AFAIS. Mitigation: Send Ether using call instead of transfer.

8. RewardDistributor addFee: more meaningful input names

Description: the function takes an array of 2 unnamed addresses as input:

    function addFee(address[2] memory addr, uint256 fee) public onlyTrader {

These are "later revealed" to be the trader address and exchange address:

        feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee;
        feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee;

As the array is only size 2, there's not much point in sending it as an array. The input is unnamed making it harder to understand to understand what is it. And the comment also is not detailed enough to understand. Mitigation: instead of the array, take 2 parameters traderAddress and exchangeAddress as input.

9. Comments issues

a. fillCriteriaBid is missing some parameters from the NATSPEC comments. b. _settleBalances comment mentions that the Order struct to be filled must be orderType 1 but it can also be orderType 2. c. GolomToken unfinished comment - Explain to an end user what this does. d. RewardDistributor addFee has a comment of a console.log command which can be removed. e. RewardDistributor stakerRewards returns all the epochs, not just the unclaimed ones, unlike what the comment says. f. RewardDistributor traderRewards and exchangeRewards describe the functions and the parameter wrong

10. Some requires have no revert string

Many requires in the system have meaningful revert messages, but some do not. This will make it harder for users to discern what's the problem. Consider adding a revert message to these requires. a. Reserved address check in fillAsk, fillBid, fillCriteriaBid b. o.totalAmt check in fillBid, fillCriteriaBid c. Order type, order validness, amount remaining in fillBid, fillCriteriaBid d. msg.sender check in cancelOrder e. Valid epoch check in traderClaim, exchangeClaim f. Basically all of VECore though I understand it's a fork g. Attachment and approval check in VEDelegation

11. VoteEscrowCore balanceOfNFTAt does not check ownership change

Description: we can see the balanceOfNFT function returns 0 if the NFT's ownership was changed in the current block, balanceOfNFTAt does not do that. Although balanceOfNFTAt is not used in the code at the moment, this might cause issues if it'll later be used or used offline. Mitigation: Add to balanceOfNFTAt:

        if (ownership_change[_tokenId] == _t) return 0;

12. Ungraceful underflow when calling removeDelegation with no checkpoints

Description: removeDelegation tries to get the latest checkpoint:

        uint256 nCheckpoints = numCheckpoints[tokenId];
        Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];

If there are no checkpoints, it will underflow and revert. User might not be sure what is the problem. Mitigation: add require(nCheckpoints > 0, "not delegated");.

13. No timelock for changeMinVotingPower

Description: changeMinVotingPower looks like a critical function for governance. Owner can change it without a timelock, possibly influencing governance results. Timelocks are used in various other places throughout the protocol. Mitigation: Consider adding a timelock to the function.

14. Unused variables in RewardDistributor

rewardLP and claimedUpto are not used; consider removing them for cleanliness and clearness.

15. RewardDistributor - No logs emitted, no convenient way for user to get his unclaimed epochs

Description: Upon claiming of fees for an epoch, no logs are emitted. There is no function for the user to easily get his unclaimed epochs. He can only do so by quering the public getter for each day - can become a little heavy after few years. Impact: Not easy to get the unclaimed epochs. Front ends would have to query the contract possibly even 1000 times. As I mentioned in my gas report, if user submits epochs which he has no fees with, he'll have to pay 4200 gas for each epoch. Mitigation: add logs upon claiming of fees. Consider adding a view function that will return the unclaimed epochs for the user.

16. Insufficient order amounts checks in fill bids functions

These functions try to make sure that the total amount that will get pulled from the user is enough to cover all the various fees. In fillBid:

        require(
            o.totalAmt * amount >
                (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt
        ); // cause bidder eth is paying for seller payment p , dont take anything extra from seller

In fillCriteriaBid:

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

The problems: a. Both functions do not account for the protocol fee - 50BPS. b. fillCriteriaBid's check is missing the check for p.paymentAmt. Impact: not critical as far as I can see. If the o.totalAmt is not enough to cover for the fees, the transfers would anyway fail in _settleBalances, as not enough funds were pulled from the user. The transfers would attempt to transfer the contract's ether/dust, but it will probably not be enough to cover costs. This does allow a user to sweep Ether out of the contract by filling a criteria bid with 0 amount and p that amounts to the amount to be sweeped. But generally there shouldn't be significant Ether in the contract. Mitigation: add the 50BPS fee to both checks and p.paymentAmt to fillCriteriaBid.

I present a few issues that will save substantial amount of gas.

1. Save epochs claimed in a bitmap, not separately

This change alone will save your users 100,000s of gas. 660,000 gas if claiming fees for a whole month at once.

RewardDistribution saves each claimed epoch in a separate variable. This is extremely expensive. By combining epochs to a uint bitmap, we save huge amount of gas.

Description: The problem is present in multiStakerClaim. For each epoch, the following is done:

claimed[tokenids[tindex]][epochs[index]] = 1;

Saving a new storage slot takes roughly 22100 gas units, and the current function saves a new storage slot for every claimed epoch (day). This means that claiming fees for a month will cost users more than 660,000 gas. This is a huge amount which can translate to hundreds of dollars. This might be even considered a medium severity issue as it greatly impacts the availability and usability of protocol.

Mitigation: We can save much of that gas by packing the claimed epochs into a bitmap. Instead of saving each epoch's claimed boolean in his own uint variable (current implementation):

mapping(uint256 => mapping(uint256 => uint256)) public claimed; // epoch upto which tokenid has claimed

We'll use a mapping, such that every token has it's own mapping(uint256 => uint256). For an epoch e, the key represents e/256, and in the value, each bit represents the given epoch - e/256*256 + e%256.

mapping (uint256 => mapping(uint256 => uint256)) public claimed; // bitmap: (tokenId => epoch/256 => epoch%256)
// for epoch e the claimed boolean is (claimed[tokenId][e/256] & (0x1 << e%256))

So to check if an epoch has been claimed, we'll check:

if (claimed[tokenId][e/256] & (0x1 << e%256)) == 1)

And to set an epoch as claimed, we'll set:

claimed[tokenId][e/256] = claimed[tokenId][e/256] | (0x1 << e%256)

Mitigation savings: Updating a dirty (touched) storage slot is only 100 gas, and previously it cost us roughly 22100 to set each epoch as claimed. Therefore we save roughly 20000 gas per epoch, which translates to 600,000 gas if claiming a whole month at once.

Cache external calls

Description: external calls which are done a few times without the state changing can be saved to memory and read from there. While it's only 100 gas to call a warm address or storage slot, caching them can still save few hundred gas per call. Instances: a. addFee: cache the 4 totalSupply calls (3 done when starting new epoch) and 2 balanceOf calls. b. multiStakerClaim: cache the balanceOfAtNFT and totalSupplyAt calls, each done twice. Note that these functions do many storage reads.

Skip calculation in trader and exchange fees if no fees are there

Description: for every user supplied epoch, the functions calculate the fees due, even if the user's share is set to 0:

                (rewardTrader[epochs[index]] * feesTrader[addr][epochs[index]]) /
                epochTotalFee[epochs[index]];

If a user has no fees for an epoch, this calculation still reads 2 unnecessary cold storage slots (rewardTrader and epochTotalFee), for a cost of 4200 (nice). While the user supplies his own epochs, this 4200-gas-per-epoch can add up quickly to a large amount. Mitigation: consider skipping the iteration if feesTrader[addr][epochs[index]] == 0. Note: there is no convenient way for the user to get his unclaimed epochs other than iterating on the public getter of claimed.

Cache in memory repeatedly accessed storage variables

Description: Reading a dirty storage slot is 100 gas, while reading and setting memory variables is roughly 3 gas. Therefore if there are repeated storage readings, it is efficient to save a local variable in memory and read that instead.

Instances: a. addFee: epoch is being read 7 times when adding a fee and 14 times when also starting a new epoch. Therefore saving epoch to a local variable will save on each trade roughly 600 and 1300 gas respectively. b. rewardToken is read 7 times when starting a new epoch, so caching it will save roughly 600 gas.

c. _settleBalances: WETH is read twice, so caching it should save little less than 100 gas for each fill bid trade.

d. multiStakerClaim: each epoch, ve is being read 4 times and once every tokenId. Caching should it should save roughly 300 gas per epoch. Can translate to 9,000 gas if claiming for a whole month. e. epochBeginTime[epochs[index]] is being read 4 times per epoch. Caching should save roughly 300 gas per epoch. f. epochs[index] would be read 5 times per epoch (if you do the previous change with epochBeginTime). Caching should save roughly 400 gas per epoch. g. epoch (current epoch) is being read once per iterated-epoch, so caching it should save roughly 100 gas per epoch. In total, these changes would save roughly 1,100 gas per epoch, which means 33,000 gas saved if user is claiming for whole month.