Golom contest - Maxime's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L459-L461 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L313-L316

Vulnerability details

Impact

In the event someone wrongfully sends ETH to both contract GolomTrader or Reward Distributor (for example through a low level call with the wrong parameters) fallback() or receive() functions will receive the call with the ETH in it. However ETH will be stuck in the contract since there are no withdraw functions

Recommended Mitigation Steps

In order to avoid that, I suggest:

• removing the fallback/receive function
• or adding revert() inside them.
• or creating a withdraw function that will let owner of the smart contract withdraw the ETH inside:

  function withdraw(address payable _receiver) external onlyOwner {
    _receiver.transfer(address(this).balance);
  }

Require error message too long

Require error message is over 32 bytes and will cost most more VEDelegation: Need more voting power. I suggest making it shorter by changing it to Need more voting power

        File: contracts/vote-escrow/VoteEscrowDelegation.sol   #73

        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');

Works also :

    File: contracts/rewards/RewardDistributor.sol   #181

    require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');

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

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

Should use calldata parameters instead of memory

Since the _delegatedTokenIds is not modified we could use it as an calldata parameter instead of memory inside the _writeCheckpoint function.

    File: contracts/vote-escrow/VoteEscrowDelegation.sol   #97

    function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {

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

Store storage variable inside memory variable to reduce gas cost

The checkpoints[toTokenId] is potentially called 3 times within the same _writeCheckpoint so I believe we could potentially store it inside a memory variable in order to reduce gas cost every time it is called and then update the variable in storage accordingly.

      File: contracts/vote-escrow/VoteEscrowDelegation.sol   #101
      Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

     File: contracts/vote-escrow/VoteEscrowDelegation.sol   #106
     checkpoints[toTokenId][nCheckpoints]

     File: contracts/vote-escrow/VoteEscrowDelegation.sol   #107
     checkpoints[toTokenId][nCheckpoints]
     numCheckpoints[toTokenId] = nCheckpoints + 1

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

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

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

Optimise for loops

In order to optimise for loops, I would advise storing .length method's return inside a memory variable. Also we can assume that the iterator i will not overflow so it is safe to use the uncheck attribute and replace it by ++i in order to reduce useless opcodes and reduce gas cost.

      File: contracts/vote-escrow/VoteEscrowDelegation.sol   #171

        for (uint256 index = 0; index < delegated.length; index++) {
            votes = votes + this.balanceOfNFT(delegated[index]);
        }

could be replaced by

       uint256 delegatedLength = delegated.length;
        for (uint256 index = 0; index < delegatedLength;) {
            votes = votes + this.balanceOfNFT(delegated[index]);
            unchecked {++i}
        }

Same for:

      File: contracts/vote-escrow/VoteEscrowDelegation.sol   #189

        for (uint256 index = 0; index < delegatednft.length; index++) {
            votes = votes + this.balanceOfAtNFT(delegatednft[index], blockNumber);
        }

could be replaced by

       uint256 delegatedNftLength = delegatednft.length;

        for (uint256 index = 0; index < delegatedNftLength;) {
            votes = votes + this.balanceOfAtNFT(delegatednft[index], blockNumber);
            unchecked { index++} 
        }

      File: contracts/vote-escrow/VoteEscrowDelegation.sol   #199

        for (uint256 i; i < _array.length; i++) {
            if (_array[i] == _element) {
                _array[i] = _array[_array.length - 1];
                _array.pop();
                break;
            }
        }

could be replaced by

        uint256 arrayLength = _array.length; 

        for (uint256 i; i < arrayLength;) {
            if (_array[i] == _element) {
                _array[i] = _array[arrayLength - 1];
                _array.pop();
                break;
            }
            uncheck { +ii} 
        }

     File: contracts/rewards/RewardDistributor.sol   #156

        uint256 reward = 0;
        for (uint256 index = 0; index < epochs.length; index++) {
            require(epochs[index] < epoch);
            reward =
                reward +
                (rewardExchange[epochs[index]] * feesExchange[addr][epochs[index]]) /
                epochTotalFee[epochs[index]];
            feesExchange[addr][epochs[index]] = 0;
        }

could be replace by

        uint256 epochsLength = epochs.length;

        for (uint256 index = 0; index < epochsLength;) {
            require(epochs[index] < epoch);
            reward =
                reward +
                (rewardExchange[epochs[index]] * feesExchange[addr][epochs[index]]) /
                epochTotalFee[epochs[index]];
            feesExchange[addr][epochs[index]] = 0;
          uncheck {++index}
        }

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

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

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

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

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

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

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

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

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

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

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

Change order in which way the require is located

In order to reduce gas cost for users that will not pass the following require : require(_isApprovedOrOwner(_sender, _tokenId)) I believe we should move up this require before the this.removeDelegation(_tokenId); since this function is pretty costly and has no impact on the next require.

    File: contracts/vote-escrow/VoteEscrowDelegation.sol   #245

    require(_isApprovedOrOwner(_sender, _tokenId))

should be before

    File: contracts/vote-escrow/VoteEscrowDelegation.sol   #242

    this.removeDelegation(_tokenId)

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

Cheaper to not instantiate storage variable by 0

It cheaper to not instantiate variable by 0 and let the EVM automatically set it to 0

    File: contracts/rewards/RewardDistributor.sol   #45

     uint256 public epoch = 0;

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

Repack Order struct in order to use less storage slots

In order to make the Order struct more gas efficient we should rethink the way it is packed. For that we have the move the uint8 v next to a 20 bytes address like address collection or reservedAddress so that instead of respectively take 2 slots they will only use one.

The initial

     File: contracts/core/GolomTrader.sol   #47

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

could be changed to:

    struct Order {
        address collection; // NFT contract address
        uint8 v;
        uint256 tokenId; // order for which tokenId of the collection
        address signer; // maker of order address
        uint256 orderType; 
        uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount
        Payment exchange; 
        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;
        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
        bytes32 r;
        bytes32 s;
    }

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

Use abi.encodePacked over abi.encode

It is more efficient to use the abi.encodePacked over the abi.encode method and should be used in this case since we will then hash the result of this method into a 32 bytes variable using the keccak256 function

       File: contracts/core/GolomTrader.sol   #102

        EIP712_DOMAIN_TYPEHASH = keccak256(
            abi.encode(
                keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'),
                keccak256(bytes('GOLOM.IO')),
                keccak256(bytes('1')),
                chainId,
                address(this)
            )
        );

Same for:

            File: contracts/core/GolomTrader.sol   #114

            keccak256(
                abi.encode(
                    keccak256('payment(uint256 paymentAmt,address paymentAddress)'),
                    p.paymentAmt,
                    p.paymentAddress
                )
            );

            File: contracts/core/GolomTrader.sol   #129

            keccak256(
                abi.encode(
                    keccak256(
                        'order(address collection,uint256 tokenId,address signer,uint256 orderType,uint256 totalAmt,payment exchange,payment prePayment,bool isERC721,uint256 tokenAmt,uint256 refererrAmt,bytes32 root,address reservedAddress,uint256 nonce,uint256 deadline)payment(uint256 paymentAmt,address paymentAddress)'
                    ),
                    o.collection,
                    o.tokenId,
                    o.signer,
                    o.orderType,
                    o.totalAmt,
                    hashPayment(o.exchange),
                    hashPayment(o.prePayment),
                    o.isERC721,
                    o.tokenAmt,
                    o.refererrAmt,
                    o.root,
                    o.reservedAddress,
                    extra
                )
            );

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

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

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