Golom contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

On the initial RewardDistributor.addVoteEscrow call, the owner of the contract can set the ve address without a timelock (which is as intended according to the function documentation). However, as the function parameter _voteEscrow is not used for the assignment, instead the storage variable pendingVoteEscrow (which is not initialized, hence address(0)) is used, the ve storage variable can not be set to the provided _voteEscrow address.

This prevents setting the ve address (ve is set to address(0)) and therefore prevents veNFT holders to claim reward tokens and Ether rewards via RewardDistributor.multiStakerClaim.

Proof of Concept

RewardDistributor.sol#L300

function addVoteEscrow(address _voteEscrow) external onlyOwner {
    if (address(ve) == address(0)) {
        ve = VE(pendingVoteEscrow); // @audit-info The wrong variable is used. It should be `_voteEscrow`
    } else {
        voteEscrowEnableDate = block.timestamp + 1 days;
        pendingVoteEscrow = _voteEscrow;
    }
}

RewardDistributor.sol#L173

function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {
    require(address(ve) != address(0), ' VE not added yet'); // @audit-info reverts if `ve` is not initialized

    ...
}

Tools Used

Manual review

Recommended mitigation steps

Use the correct function parameter _voteEscrow:

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

Lines of code

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

Vulnerability details

Impact

Delegating voting power of veNFT tokens with VoteEscrowDelegation.delegate requires storing checkpoints and the current delegation for the NFT in the specific block with VoteEscrowDelegation._writeCheckpoint.

The current implementation is faulty and does not allow delegating voting power due to the VoteEscrowDelegation._writeCheckpoint call reverting on the first delegation of a token.

Proof of Concept

vote-escrow/VoteEscrowDelegation.sol#L101

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

If nCheckpoints = 0, which is the case for the first checkpoint for a given tokenId delegated with VoteEscrowDelegation.delegate, checkpoints[toTokenId][nCheckpoints - 1] will underflow and revert due to 0 - 1. This renders vote escrow delegation unable to use as the VoteEscrowDelegation.delegate reverts on each call.

Tools Used

Manual review

Recommended mitigation steps

Only access oldCheckpoint if nCheckpoints does not equal 0:

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

    Checkpoint memory oldCheckpoint;

    if (nCheckpoints > 0) {
        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;
    }
}

Lines of code

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

Vulnerability details

Impact

A veNFT holder can delegate the voting power of the NFT with the VoteEscrowDelegation.delegate function to a token toTokenId. However, the current implementation does not check if the NFT with tokenId is already delegated to a token toTokenId. Thus, it is possible to delegate a NFT multiple times and therefore increase the voting power of delegatees infinitely.

Proof of Concept

VoteEscrowDelegation.sol#L75

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

    // @audit-info `tokenId` can be delegated to multiple delegatees at the same time as there is no check for an existing delegation

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

Tools Used

Manual review

Recommended mitigation steps

Check the mapping delegates[tokenId] for an already active vote delegation and revert if the tokenId is already delegated:

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');
    require(delegates[tokenId] != address(0), 'Already delegated'); // @audit-info add check for already existing delegation

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

Lines of code

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

Vulnerability details

Impact

Trading fees are contributed by the seller (trader) of a NFT and the exchange that facilitated the trade with the RewardDistributor.addFee function called from the GolomTrader contract. Ether fees are transferred to the RewardDistributor contract with a separate Solidity transfer call (for instance, see GolomTrader.sol#L242).

veNFT holders are then able to claim these trading Ether fees (additionally to GOLOM reward tokens). Reward token distribution will end after the max supply of 1 billion tokens is reached. However, collected Ether trading fees are continued to be collected and received from the GolomTrader contract, but not kept track of within the RewardDistributor.addFee function due to returning early. Therefore, veNFT holders can not claim collected Ether funds after the reward token limit is reached. Fees are stuck in the RewardDistributor contract.

Proof of Concept

RewardDistributor.sol#L100-L103

function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
  //console.log(block.timestamp,epoch,fee);
  if (rewardToken.totalSupply() > 1000000000 * 10**18) {
      // if supply is greater then a billion dont mint anything, dont add trades
      return; // @audit-info Returns early if max supply is reached
  }

  ...
}

RewardDistributor.multiStakerClaim

rewardEth =
  rewardEth +
  (epochTotalFee[epochs[index]] *
      ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]])) /
  ve.totalSupplyAt(epochBeginTime[epochs[index]]);

As no new epochs will exist after the maximum reward token supply is reached, rewardEth can only collect Ether fees from epochs until then.

Tools Used

Manual review

Recommended mitigation steps

Continue to keep track of received Ether trading fees within the RewardDistributor.addFee function by not returning early but stop minting and distributing reward tokens. This will allow veNFT holders to continue claiming Ether trading fees even after reaching the maximum reward token supply.

Lines of code

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

Vulnerability details

Impact

A veNFT holder can remove the current vote delegation (delegatee) for a token with the id tokenId (delegator) with the VoteEscrowDelegation.removeDelegation function.

However, the checkpoint used to remove the delegation is the checkpoint for the tokenId (delegator) instead of the delegatee. Therefore it is currently not possible to remove vote delegation for the delegator tokenId and its delegatee toTokenId.

Proof of Concept

VoteEscrowDelegation.sol#L213

/// @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]; // @audit-info Removing vote delegation removes delegation from delegator instead of delegatee
    removeElement(checkpoint.delegatedTokenIds, tokenId);
    _writeCheckpoint(tokenId, nCheckpoints, checkpoint.delegatedTokenIds);
}

Tools Used

Manual review

Recommended mitigation steps

Use the mapping delegates[tokenId] to figure out the current delegatee token id and use the checkpoint checkpoints for this delegatee to remove the tokenId from its delegatedTokenIds:

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

    uint delegateeTokenId = delegates[tokenId]; // @audit-info use current delegatee token id
    uint256 nCheckpoints = numCheckpoints[delegateeTokenId];
    Checkpoint storage checkpoint = checkpoints[delegateeTokenId][nCheckpoints - 1];
    removeElement(checkpoint.delegatedTokenIds, tokenId);
    _writeCheckpoint(delegateeTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
}

Lines of code

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

Vulnerability details

Impact

veNFT tokens may be sent to contracts that cannot handle them, and therefore all rewards and voting power, as well as the underlying are locked forever.

Proof of Concept

The original code of Solidly had the following warning:

* @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients
* are aware of the ERC721 protocol to prevent tokens from being forever locked.

https://github.com/solidlyexchange/solidly/blob/eac1ef02ca8736138b25062c02646ea43d5bb6e4/contracts/ve.sol#L143-L144

The minting code, which creates the locks, does not do this check:

VoteEscrowCore._mint

function _mint(address _to, uint256 _tokenId) internal returns (bool) {
    // Throws if `_to` is zero address
    assert(_to != address(0));
    // Add NFT. Throws if `_tokenId` is owned by someone
    _addTokenTo(_to, _tokenId);
    emit Transfer(address(0), _to, _tokenId);
    return true;
}

Once a lock is already minted, if it's transferred to a contract, the code does attempt to check for the issue:

VoteEscrowCore.safeTransferFrom

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

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

While the transfer function does in fact execute onERC721Received, it doesn't actually do a check of the bytes4 variable - it only checks for a non-zero length. The ERC721 standard says that for the function call to be valid, it must return the bytes4 function selector, otherwise it's invalid.

If a user of the escrow system uses a contract that is attempting to explicitly reject NFTs by returning zero in its onERC721Received() function, the VotingEscrowCore contract will interpret that response as success and will transfer the NFT, potentially locking it forever. If the lock is minted to a contract, no checks are done, and the NFT can be locked forever.

Tools Used

Manual review

Recommended mitigation steps

Call onERC721Received() in _mint() and ensure that the return value equals IERC721Receiver.onERC721Received.selector in both _mint() and safeTransferFrom()

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

[NC-01] Replace assembly chainid with Solidity's chainId()

Description Retrieving the current chain id via the Yul chainid expression can be replaced with the Solidity native call to chainId().

Findings GolomTrader.sol#L98

uint256 chainId;
assembly {
    chainId := chainid()
}

Recommended mitigation steps

Change the constructor code of the GolomTrader contract to the following:

constructor(address _governance) {
  // sets governance as owner
  _transferOwnership(_governance);

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

Lines of code

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

Vulnerability details

Impact

Users who are approved, but do not own a particular veNFT, are supposed to be eligible to call merge and withdraw from the NFT.

Currently _burn(), used by merge() and withdraw() to remove the NFT from the system, will revert unless the sender is the owner of NFT as the function tries to update the accounting for the sender, not the owner.

Therefore, merge() and withdraw() are permanently unavailable for any approved sender, who isn't the owner of the involved NFT.

Proof of Concept

_removeTokenFrom() requires _from to be the NFT owner as it removes _tokenId from the _from account:

VoteEscrowCore.sol#L504-L513

function _removeTokenFrom(address _from, uint256 _tokenId) internal {
    // Throws if `_from` is not the current owner
    assert(idToOwner[_tokenId] == _from);
    // Change the owner
    idToOwner[_tokenId] = address(0);
    // Update owner token index tracking
    _removeTokenFromOwnerList(_from, _tokenId);
    // Change count tracking
    ownerToNFTokenCount[_from] -= 1;
}

_burn() allows _tokenId to approved or owner, but calls _removeTokenFrom() with msg.sender as _from:

VoteEscrowCore.sol#L1234

function _burn(uint256 _tokenId) internal {
    require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

    address owner = ownerOf(_tokenId);

    // Clear approval
    approve(address(0), _tokenId);
    // Remove token
    _removeTokenFrom(msg.sender, _tokenId);
    emit Transfer(owner, address(0), _tokenId);
}

This way if _burn() is called by an approved account who isn't an owner, it will revert on _removeTokenFrom()'s assert(idToOwner[_tokenId] == _from); check. And _burn() is called by merge() and withdraw().

Tools Used

Manual review

Recommended mitigation steps

Consider changing VoteEscrowCore._removeTokenFrom() argument to be the owner instead of msg.sender:

function _burn(uint256 _tokenId) internal {
    require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

    address owner = ownerOf(_tokenId);

    // Clear approval
    approve(address(0), _tokenId);
    // Remove token
-   _removeTokenFrom(msg.sender, _tokenId);
+   _removeTokenFrom(owner, _tokenId);
    emit Transfer(owner, address(0), _tokenId);
}

Lines of code

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

Vulnerability details

Impact

veNFT holders can get unlimited votes which leads them to gain undeserved control over governance.

Proof of Concept

VoteEscrowCore.sol#L1234

function _burn(uint256 _tokenId) internal {
    require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

    address owner = ownerOf(_tokenId);

    // Clear approval
    approve(address(0), _tokenId);
    // Remove token
    _removeTokenFrom(msg.sender, _tokenId); // @audit-info Burning a veNFT does not remove current delegation
    emit Transfer(owner, address(0), _tokenId);
}

A user can deposit a GOLOM token, lock it to receive a veNFT, delegate it to another token, wait for the lock to expire, withdraw the veNFT to burn it and repeat. During each iteration, a new veNFT NFT is minted and checkpointed after delegating voting power via VoteEscrowDelegation.delegate.

Since VoteEscrowCore._burn does not clear the current vote delegation, calls to VoteEscrowDelegation.getVotes and VoteEscrowDelegation.getPriorVotes will show the wrong values, since it will think the veNFT token still holds the delegation of the burnt NFT.

Tools Used

Manual review

Recommended mitigation steps

The VoteEscrowDelegation contract should also override the VoteEscrowCore._removeTokenFrom function and remove the current voting delegation (similar to how it is done in the VoteEscrowDelegation._transferFrom function on L242):

function _removeTokenFrom(address _from, uint256 _tokenId) internal {
    // Throws if `_from` is not the current owner
    assert(idToOwner[_tokenId] == _from);

    // remove the delegation
    this.removeDelegation(_tokenId); // @audit-info Remove the current delegation

    // Change the owner
    idToOwner[_tokenId] = address(0);
    // Update owner token index tracking
    _removeTokenFromOwnerList(_from, _tokenId);
    // Change count tracking
    ownerToNFTokenCount[_from] -= 1;
}

Lines of code

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

Vulnerability details

Impact

GolomTrader.sol

On the initial GolomTrader.setDistributor call, the owner of the contract can set a distributor without a timelock (which is as intended according to the function documentation). However, as the distributorEnableDate and the pendingDistributor storage variables are not initialized, hence distributorEnableDate = 0 and pendingDistributor = address(0), the owner can call GolomTrader.executeSetDistributor at anytime without any timelock to set the distributor address to the zero address again. Afterwards the owner can call GolomTrader.setDistributor once again with an arbitrary _distributor address without any timelock enforced (due to address(distributor) == address(0)).

Tl;dr: The contract owner can change the distributor to any address at any time without any timelock even after setting it to an address with setDistributor initially.

Impact:

• If distributor is set to address(0), filling orders is not possible due to reverting function calls
• If distributor is set to any arbitrary (non address(0)) address, controlled by the owner of the protocol (or if an attacker was able to get access to the owner's private keys), trading fees can be stolen and forfeit from veNFT holders

RewardDistributor.sol

Similar issues as above mentioned for the GolomTrader contract affecting the RewardDistributor.addVoteEscrow and RewardDistributor.executeAddVoteEscrow functions.

Impact:

• If ve is set to address(0), the owner can forfeit veNFT staking rewards (see L220)

Proof of Concept

GolomTrader.sol#L455

/// @notice Sets the distributor contract
/// @param _distributor Address of the distributor
function setDistributor(address _distributor) external onlyOwner {
    if (address(distributor) == address(0)) {
        distributor = Distributor(_distributor);
    } else {
        pendingDistributor = _distributor;
        distributorEnableDate = block.timestamp + 1 days;
    }
}

/// @notice Executes the set distributor function after the timelock
function executeSetDistributor() external onlyOwner {
    require(distributorEnableDate <= block.timestamp, 'not allowed');
    distributor = Distributor(pendingDistributor);
}

RewardDistributor.sol#L309

/// @notice Adds vote escrow contract for multi staker claim
/// @param _voteEscrow Address of the voteEscrow contract
function addVoteEscrow(address _voteEscrow) external onlyOwner {
    if (address(ve) == address(0)) {
        ve = VE(pendingVoteEscrow);
    } else {
        voteEscrowEnableDate = block.timestamp + 1 days;
        pendingVoteEscrow = _voteEscrow;
    }
}

/// @notice Adds vote escrow contract for multi staker claim
function executeAddVoteEscrow() external onlyOwner {
    require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
    ve = VE(pendingVoteEscrow);
}

Tools Used

Manual review

Recommended mitigation steps

GolomTrader.sol

Prevent setting distributor to address(0) in the GolomTrader.executeSetDistributor function (see @audit-info annotation):

function executeSetDistributor() external onlyOwner {
    require(pendingDistributor != address(0), 'Pending distributor not set'); // @audit-info add check
    require(distributorEnableDate <= block.timestamp, 'not allowed');
    distributor = Distributor(pendingDistributor);
}

RewardDistributor.sol

Prevent setting ve to address(0) in the RewardDistributor.executeAddVoteEscrow function (see @audit-info annotation):

function executeAddVoteEscrow() external onlyOwner {
    require(pendingVoteEscrow != address(0), 'Pending vote escrow address not set'); // @audit-info add check
    require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');
    ve = VE(pendingVoteEscrow);
}

Table of Contents

• Non-Critical Findings
  • [NC-01] Replace assembly chainid with Solidity's chainId()
  • [NC-02] EIP712_DOMAIN_TYPEHASH should be named DOMAIN_SEPERATOR
  • [NC-03] unclaimedepochs in RewardDistributor.stakerRewards should contain reward amounts per epoch
  • [NC-04] Lack of revert messages
• Low Risk
  • [L-01] chainid may change in case of a hardfork
  • [L-02] Incorrect require statement in GolomTrader.validateOrder
  • [L-03] Maximum total supply of GOLLOM tokens can surpass limit of 1 billion tokens
  • [L-04] Incorrect usage of ERC20 token interface for WETH token
  • [L-05] Restrict ETH funds receivable by contracts
  • [L-06] Events not emitted for important state changes
  • [L-07] OrderFilled events can be spammed with amount = 0
  • [L-08] Outdated Solidity compiler
  • [L-09] Anyone can claim rewards for traders, exchanges and stakers on behalf of someone else

Non-Critical Findings

[NC-01] Replace assembly chainid with Solidity's chainId()

Description

Retrieving the current chain id via the Yul chainid expression can be replaced with the Solidity native call to chainId().

Findings

GolomTrader.sol#L98

uint256 chainId;
assembly {
    chainId := chainid()
}

Recommended mitigation steps

Change the constructor code of the GolomTrader contract to the following:

constructor(address _governance) {
  // sets governance as owner
  _transferOwnership(_governance);

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

[NC-02] EIP712_DOMAIN_TYPEHASH should be named DOMAIN_SEPERATOR

Description

The immutable storage variable EIP712_DOMAIN_TYPEHASH in the GolomTrader contract is misleading as it is the domain separator. The domain typehash would be keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)').

Findings

GolomTrader.sol#L101

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

Recommended mitigation steps

Consider changing the variable name from EIP712_DOMAIN_TYPEHASH to something like DOMAIN_SEPERATOR.

[NC-03] unclaimedepochs in RewardDistributor.stakerRewards should contain reward amounts per epoch

Description

unclaimedepochs should store the reward amounts per epoch to easily find out which epochs should be claimed to prevent calling reward harvest functions with epochs that do not hold any rewards for the given address

Findings

RewardDistributor.sol#L227

Recommended mitigation steps

Consider returning the reward amounts per epoch in the unclaimedepochs variable.

[NC-04] Lack of revert messages

Description

Missing revert messages can cause issues for the users who can't determine the cause for their failing transactions.

Findings

core/GolomTrader.sol

L220 - require(msg.sender == o.reservedAddress);
L291 - require(msg.sender == o.reservedAddress);
L293 - require(o.orderType == 1);
L295 - require(status == 3);
L296 - require(amountRemaining >= amount);
L313 - require(o.signer == msg.sender);
L342 - require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
L345 - require(msg.sender == o.reservedAddress);
L347 - require(o.orderType == 2);
L349 - require(status == 3);
L350 - require(amountRemaining >= amount);

rewards/RewardDistributor.sol

L88 - require(msg.sender == trader);
L144 - require(epochs[index] < epoch);
L158 - require(epochs[index] < epoch);

vote-escrow/VoteEscrowCore.sol

L360 - require(_entered_state == _not_entered);
L646 - require(owner != address(0));
L648 - require(_approved != owner);
L652 - require(senderIsOwner || senderIsApprovedForAll);
L869 - require(msg.sender == voter);
L874 - require(msg.sender == voter);
L879 - require(msg.sender == voter);
L884 - require(msg.sender == voter);
L889 - require(msg.sender == voter);
L895 - require(_from != _to);
L927 - require(_value > 0);
L944 - require(_value > 0);

Recommended mitigation steps

Consider adding revert messages where missing.

Low Risk

[L-01] chainid may change in case of a hardfork

Description

The domainSeperator (which in fact is the EIP712_DOMAIN_TYPEHASH variable in the GolomTrader contract, just wrongly named btw) is not recalculated in the case of a hard fork. The immutable variable is cached in the contract storage and will not change after being initialized. However, in the event of a hard fork, the domain would become invalid on one of the forked chains due to the change of chainid.

Findings

GolomTrader.sol#L101-L109

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

Recommended mitigation steps

Consider using the elegant solution provided by OpenZeppelin:

https://github.com/fractional-company/contracts/blob/master/src/OpenZeppelin/drafts/EIP712.sol

[L-02] Incorrect require statement in GolomTrader.validateOrder

Description

According to the function NatSpec documentation, if the signature is invalid, the returned order status should be 0 instead of reverting.

Findings

GolomTrader.sol#L177

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

Recommended mitigation steps

Remove the require statement as the signaturesigner is already checked in the if following the require.

[L-03] Maximum total supply of GOLLOM tokens can surpass limit of 1 billion tokens

Description

There is a maximum GOLOM reward token supply limit of 1 billion tokens intended by the project authors (see L101). However, the limit is incorrectly enforced as it does not factor in the amount of tokens to be emitted in the current epoch. Hence, it is possible to surpass the limit of 1 billion tokens.

Findings

rewards/RewardDistributor.addFee#L100

if (rewardToken.totalSupply() > 1000000000 * 10**18) {
    // if supply is greater then a billion dont mint anything, dont add trades
    return;
}

Recommended mitigation steps

Consider calculating tokenToEmit before the if statement and use it to enforce supply limit by checking rewardToken.totalSupply() + tokenToEmit > 1000000000 * 10**18:

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

if (rewardToken.totalSupply() + tokenToEmit > 1000000000 * 10**18) {
    // if supply is greater then a billion dont mint anything, dont add trades
    return;
}

[L-04] Incorrect usage of ERC20 token interface for WETH token

Description

There are instances across the contracts where an incorrect interface is used for WETH tokens. This does currently not present any issues, but as there are methods defined in the interface which are not available on the deployed WETH contract, it could cause future issues when calling unavailable methods.

Findings

core/GolomTrader.sol#L45

ERC20 WETH = ERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);

rewards/RewardDistributor.sol#L69

ERC20 public weth;

Recommended mitigation steps

Consider using the original WETH interface.

[L-05] Restrict ETH funds receivable by contracts

Description

Native Ether transfers to contracts should be, if possible, limited to a limited set of contracts to prevent accidental native fund transfers from other sources.

Findings

core/GolomTrader.sol#L459
core/GolomTrader.sol#L461
rewards/RewardDistributor.sol#L313
rewards/RewardDistributor.sol#L315

Recommended mitigation steps

Modify the receive() and fallback functions to only accept transfers from the expected contracts. For instance:

receive() external payable {
  require(msg.sender == wethAddress, 'Only WETH');
}

fallback() external payable {
  require(msg.sender == wethAddress, 'Only WETH');
}

[L-06] Events not emitted for important state changes

Description

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings

Many occurrences across all contracts where functions modify state variables without emitting events.

Recommended mitigation steps

Emit events for state variable changes.

[L-07] OrderFilled events can be spammed with amount = 0

Description

The GolomTrader.fillAsk, GolomTrader.fillBid and GolomTrader.fillCriteriaBid functions emit the OrderFilled event whenever an order is filled. However, by providing the function argument amount = 0, no transfers take place, but the event is still emitted. This can cause confusion and spams off-chain event monitoring tools.

Findings

GolomTrader.sol#L205
GolomTrader.sol#L281
GolomTrader.sol#L336

Recommended mitigation steps

Consider adding a check to ensure amount is larger than 0:

require(amount > 0, 'Amount can not be 0');

[L-08] Outdated Solidity compiler

Description

The pragma version used is:

pragma solidity 0.8.11;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.
• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa (see official blog post for more infos).

Apart from these, there are several minor bug fixes and improvements.

Findings

GolomTrader.sol#L3

Recommended mitigation steps

The minimum required version should be 0.8.14

[L-09] Anyone can claim rewards for traders, exchanges and stakers on behalf of someone else

Description

The RewardDistributor contract implements multiple functions to claim rewards for various actors (traders, exchanges and stakers). Those functions are callable by anyone on behalf of someone else.

While the tokens are sent to the correct receiver, this can lead to issues with smart contracts that might rely on claiming the rewards themselves.

If the receiver contract has no other functions to transfer out funds, they may be locked forever in this contract.

Claiming can also incur a taxable event and the timing is better left to the actual owner.

Findings

RewardDistributor.sol#L141 - function traderClaim(address addr, uint256[] memory epochs)
RewardDistributor.sol#L155 - function exchangeClaim(address addr, uint256[] memory epochs)
RewardDistributor.sol#L172 - function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs)

Recommended mitigation steps

Do not allow users to claim on behalf of other addresses by using msg.sender.