Golom contest - shenwilly's results

Lines of code

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

Vulnerability details

Impact

RewardDistributor will stop accumulating fees for staker rewards once rewardToken supply has reached the maximum supply (1 billion).

Vulnerability Details

RewardDistributor.sol#L98-L138

function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
    if (rewardToken.totalSupply() > 1000000000 * 10**18) {
        // if supply is greater then a billion dont mint anything, dont add trades
        return;
    }
    
    ...
    
    feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee;
    feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee;
    epochTotalFee[epoch] = epochTotalFee[epoch] + fee;
}

The check at the beginning of addFee is supposed to stop RewardDistributor from minting additional rewardToken once it has reached 1 billion supply. However, the current implementation has a side effect of causing the function to skip recording accumulated trading fees (the last 3 lines of the function). This will cause stakers to lose their trading fee rewards once the max supply has been reached, and the funds will be permanently locked in the contract.

Proof of Concept

• Alice staked GOLOM to receive fee rewards from RewardDistributor.
• GOLOM supply reaches 1 billion token.
• Traders keep trading on GolomTrader, sending protocol fees to RewardDistributor. However, RewardDistributor.addFee does not update the fee accounting.
• Alice won't receive any fee reward and protocol fees are stuck in the contract.

Recommended Mitigation Steps

Modify addFee so that the check won't skip accruing trade fees:

function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
    if (block.timestamp > startTime + (epoch) * secsInDay) {
        uint256 previousEpochFee = epochTotalFee[epoch];
        epoch = epoch + 1;

        if (rewardToken.totalSupply() > 1000000000 * 10**18) {
            emit NewEpoch(epoch, 0, 0, previousEpochFee);
        } else {
            uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
                rewardToken.totalSupply();
            uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();

            rewardStaker[epoch] = stakerReward;
            rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
            rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;
            rewardToken.mint(address(this), tokenToEmit);
            epochBeginTime[epoch] = block.number;
            if (previousEpochFee > 0) {
                if (epoch == 1){
                    epochTotalFee[0] =  address(this).balance; // staking and trading rewards start at epoch 1, for epoch 0 all contract ETH balance is converted to staker rewards rewards.
                    weth.deposit{value: address(this).balance}();  
                }else{
                    weth.deposit{value: previousEpochFee}();
                }
            }
            emit NewEpoch(epoch, tokenToEmit, stakerReward, previousEpochFee);
        }
    }
    feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee;
    feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee;
    epochTotalFee[epoch] = epochTotalFee[epoch] + fee;
    return;
}

Lines of code

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

Vulnerability details

Impact

transfer has a fixed 2300 gas stipend, which may not be enough if the recipient is a contract that needs more than 2300 gas to receive ETH.

Even for contracts that currently consume less than 2300 gas to receive ETH, it is possible that future Ethereum upgrades could change the gas cost calculation and break these contracts. Using call will prevent these scenarios.

Reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Recommended Mitigation Steps

Consider using call instead of transfer.

Lines of code

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

Vulnerability details

Impact

Contract recipients might not be able to handle ERC721 tokens sent via transferFrom, leading to irregular state or potential loss of token in the contract.

Vulnerability Details

Some contracts expect and have custom logic that is triggered whenever they receive ERC721 tokens via the ERC721TokenReceiver.onERC721Received standard. They might not be able to handle unexpected tokens transferred via transferFrom, potentially leading to tokens getting stuck in these contracts.

Recommended Mitigation Steps

If contracts are valid users of the protocol, use the safeTransferFrom variant when transferring ERC721 tokens.

Low Risk Vulnerabilities

1. Guard against chain forks

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

If a hard fork happened after contract deployment, the EIP712_DOMAIN_TYPEHASH on the forked chain would become invalid because block.chainid of the new chain has changed. This will cause unfilled orders to be fillable on all forked chains.

Recommended Mitigation Steps

Consider using OpenZeppelin's implementation of domainSeparator which recalculates the typeHash if the current block.chainid is not the cached chainId.

2. Governor quorum and threshold need adjustments

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GovernerBravo.sol#L14-L16 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GovernerBravo.sol#L19-L21

GovernerBravo.sol is forked from Compound and has the assumption that the maximum supply for the governance token is 10 million token:

• quorumVotes: the minimum amount of vote needed to reach a quorum pass a proposal. Hard-coded at 400,000 (4% of total COMP supply).
• proposalThreshold: the number of vote required to become a proposer. Hard-coded at 100,000 (1% of total COMP supply).

GOLOM, however, will have 200 million token distributed initially, with a maximum supply of 1 billion token. Any user will only need a very small amount 100,000/200,000,000 = 0,05% of token supply to become a proposer.

The GovernerBravo.sol contract is out of scope, but we decided to report it nonetheless as vulnerabilities on the governance contract could affect contracts within scope.

Recommended Mitigation Steps

Adjust the value of quorumVotes and proposalThreshold to a reasonable value.

3. Adopt a more flexible nonce model

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

Incrementing nonce via incrementNonce will invalidate every current standing orders, as order validation will require the order's nonce to match the current nonce of the user: GolomTrader.sol#L185-L188

// not cancelled by nonce or by hash
if (o.nonce != nonces[o.signer]) {
    return (2, hashStruct, 0);
}

It is possible for a trader to have a lot of standing orders, and only want to cancel several of them. With the current implementation, they will be forced to increment nonce and re-sign all the remaining orders, as cancelling them one by one via cancelOrder is expensive. This is a UX burden on the user.

A better approach would be to disallow orders that have lower nonce than the current user nonce. This will allow more flexible strategies for the trader and reduce the amount of re-signing of orders. For example:

• Alice sign ten orders with increasing nonces from 1 to 9. If Alice only want to cancel the 3 oldest orders, she can increase the current nonce from 0 to 4 to revoke orders with nonce lower than 4.
• Alice sign a group of orders with nonce 0 and a group of orders with nonce 1. Alice will have both order groups available and has the option to cancel the nonce 0 group.

Recommended Mitigation Steps

Consider modifying how the nonce validation works, from:

if (o.nonce != nonces[o.signer]) {
    return (2, hashStruct, 0);
}

to:

if (o.nonce < nonces[o.signer]) {
    return (2, hashStruct, 0);
}

In addition, consider adding a target nonce parameter in incrementNonce() to allow cancelling multiple nonce at once:

function incrementNonce(uint256 nonce) external nonReentrant {
    require(nonce > nonces[msg.sender]);
    nonces[msg.sender] = nonce;
    emit NonceIncremented(msg.sender, nonces[msg.sender]);
}

4. Filling ERC1155 order with 0 amount will still pass

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

When filling an ERC1155 order, it is possible for the amount param to be 0 and the transaction will still succeed. While there is no funds at risk when this happens, this issue could generate unnecessary OrderFilled event emission and potentially disrupt off-chain analytics that rely on this event.

Recommended Mitigation Steps

Require the amount to be greater than zero when token is ERC1155:

if (o.isERC721) {
    require(amount == 1, 'only 1 erc721 at 1 time');
    ...
} else {
    require(amount > 0, 'amount must not be 0');
    ...
}

5. Wrong NatSpec comments

The NatSpec comments of these functions are wrong:

• traderRewards() The comment should be:

  /// @dev returns unclaimed golom rewards of a trader
  /// @param addr the trader address to check
  

• exchangeRewards() The comment should be:

  /// @dev returns unclaimed golom rewards of an exchange
  /// @param addr the exchange address to check
  

6. Inaccurate comment

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

// if royalty has to be paid
payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress

The comments in payEther mentioned that the function is intended only for transferring royalty. However, the function is actually used for other purposes in the codebase as a generic function for transferring ETH payment. Consider removing the comment.

Non-critical Vulnerabilities

1. Typos

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L140 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L154 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L168 there should be replaced to their.

2. Unused comment

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L99 This is an unused comment that can be safely removed.