Golom contest - hyh's results

Lines of code

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

Vulnerability details

It's now impossible to set non-zero ve address in RewardDistributor as addVoteEscrow() performs no operations.

This will render addFee(), which is triggered on each filled trade, to ignore stakers part of the rewards, i.e. even if there is a substantial share of staked $GOLOM, addFee() will behave as is no stakers present at all. multiStakerClaim() and stakerRewards() that properly check ve to exist, are disabled permanently.

Setting severity to be high as this is a violation of system logic and massive fund loss impact for the stakers.

Proof of Concept

ve isn't initialized on construction:

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

    VE public ve;

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

    constructor(
        address _weth,
        address _trader,
        address _token,
        address _governance
    ) {
        weth = ERC20(_weth);
        trader = _trader;
        rewardToken = ERC20(_token);
        _transferOwnership(_governance); // set the new owner
        startTime = 1659211200;
    }

addVoteEscrow() sets ve to be pendingVoteEscrow if ve is zero, but pendingVoteEscrow cannot be set while ve is zero:

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

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

addVoteEscrow() is the only place where the pendingVoteEscrow can be set.

This way any calls to addVoteEscrow() will keep ve to be zero address.

executeAddVoteEscrow() will also keep ve to remain zero address as voteEscrowEnableDate and pendingVoteEscrow are always zeros:

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

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

Empty ve address disables the most logic of RewardDistributor:

addFee():

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

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

multiStakerClaim():

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

    function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {
        require(address(ve) != address(0), ' VE not added yet');

stakerRewards():

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

    function stakerRewards(uint256 tokenid) public view returns (
            uint256,
            uint256,
            uint256[] memory
        ){
        require(address(ve) != address(0), ' VE not added yet');

Recommended Mitigation Steps

Consider setting ve to _voteEscrow instead:

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

    /// @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);
+           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/core/GolomTrader.sol#L375-L400

Vulnerability details

Currently (o.totalAmt * 50) / 10000) protocol fee share is multiplied by amount twice when being accounted for as a deduction from the total in amount due to the msg.sender taker calculations in _settleBalances(), which is called by fillBid() and fillCriteriaBid() to handle the payouts.

Setting the severity to be high as reduced payouts is a fund loss impact for taker, which receives less than it's due whenever amount > 1.

Notice that the amount lost to the taker is left on the contract balance and currently is subject to other vulnerabilities, i.e. can be easily stolen by an attacker that knowns these specifics and tracks contract state. When these issues be fixed this amount to be permanently frozen on the GolomTrader's balance as it's unaccounted for in all subsequent calculations (i.e. all the transfers are done with regard to the accounts recorded, this extra sum is unaccounted, there is no general native funds rescue function, so when all other mechanics be fixed the impact will be permanent freeze of the part of taker's funds).

Proof of Concept

_settleBalances() uses (o.totalAmt - protocolfee - ...) * amount, which is o.totalAmt * amount - ((o.totalAmt * 50) / 10000) * amount * amount - ..., counting protocol fee extra amount - 1 times:

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

            payEther(
                (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
                    amount -
                    p.paymentAmt,
                msg.sender
            );
        } else {
            payEther(
                (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
                msg.sender
            );

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

    function _settleBalances(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p
    ) internal {
        uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;
        WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);
        WETH.withdraw(o.totalAmt * amount);
        payEther(protocolfee, address(distributor));
        payEther(o.exchange.paymentAmt * amount, o.exchange.paymentAddress);
        payEther(o.prePayment.paymentAmt * amount, o.prePayment.paymentAddress);
        if (o.refererrAmt > 0 && referrer != address(0)) {
            payEther(o.refererrAmt * amount, referrer);
            payEther(
                (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
                    amount -
                    p.paymentAmt,
                msg.sender
            );
        } else {
            payEther(
                (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
                msg.sender
            );
        }

Say, if amount = 6, while ((o.totalAmt * 50) / 10000) = 1 ETH, 6 ETH is total protocolfee and needs to be removed from o.totalAmt * 6 to calculate taker's part, while 1 ETH * 6 * 6 = 36 ETH is actually removed in the calculation, i.e. 36 - 6 = 30 ETH of taker's funds will be frozen on the contract balance.

Recommended Mitigation Steps

Consider accounting for amount once, for example:

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

    function _settleBalances(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p
    ) internal {
-       uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;
+       uint256 protocolfee = ((o.totalAmt * 50) / 10000);
        WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);
        WETH.withdraw(o.totalAmt * amount);
-       payEther(protocolfee, address(distributor));
+       payEther(protocolfee * amount, address(distributor));
        payEther(o.exchange.paymentAmt * amount, o.exchange.paymentAddress);
        payEther(o.prePayment.paymentAmt * amount, o.prePayment.paymentAddress);
        if (o.refererrAmt > 0 && referrer != address(0)) {
            payEther(o.refererrAmt * amount, referrer);
            payEther(
                (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
                    amount -
                    p.paymentAmt,
                msg.sender
            );
        } else {
            payEther(
                (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
                msg.sender
            );
        }
        payEther(p.paymentAmt, p.paymentAddress);
-       distributor.addFee([msg.sender, o.exchange.paymentAddress], protocolfee);
+       distributor.addFee([msg.sender, o.exchange.paymentAddress], protocolfee * amount);
    }

Lines of code

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

Vulnerability details

GolomTrader's payEther() transfers out native tokens via payable(to).transfer call. This is unsafe as transfer has hard coded gas budget and can fail when payAddress is a smart contract. Such transactions will fail for smart contract receivers which don't fit to 2300 gas stipend transfer have.

payEther() is used for all the fund transfers to maker (in fillAsk) and taker (in fillBid and fillCriteriaBid). The affected users are smart contracts, so it is a programmatic usage failure, i.e. denial of service for such downstream system users.

Setting severity to be high as the impact is core functionality unavailability that cannot be mitigated, say if a maker is a smart contract that doesn't fit to the gas stipend then his Ask will never be filled. Besides gas costs for fill attempts there is a monetary opportunity cost of relying on an order that cannot be picked up.

Proof of Concept

payEther() performs payable.transfer kind of call to an arbitrary payAddress:

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

    function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {
            // if royalty has to be paid
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }
    }

payEther() is used for the main value transfers and the impact is partial blocking of the order filling, i.e. the protocol unavailability for certain smart contracts.

References

The issues with transfer() are outlined here:

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

Recommended Mitigation Steps

Generally replacing the transfer() should be coupled with the introduction of non-reentrancy feature, but here fillAsk(), fillBid() and fillCriteriaBid() have fund transfers as the last operations of the logic and are nonReentrant, so reentrancy isn't an issue.

This way the recommendation is to use low-level call.value(amount) with the corresponding result check or employ OpenZeppelin's Address.sendValue:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

1. Any native funds send to GolomTrader will be frozen on its balance (low)

GolomTrader has both payable fallback() and receive() functions, which do not take part in the logic, while there are no mechanics implemented to rescue mistakenly sent native tokens, so such funds will be frozen on the contract balance.

Proof of Concept

Currently the contract is able to receive ETH without invoking the logic:

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

    fallback() external payable {}

    receive() external payable {}

But such native funds will be frozen on the balance as there is no rescue function, all the funds are moved according to the filled order's parameters. Native funds attached to the call are used in fillAsk() only, while native balance aren't utilized anywhere.

Recommended Mitigation Steps

Both functions do not take part in contract logic and provide a way to freeze the funds, so consider removing fallback() and receive()

2. validateOrder's zero OrderStatus isn't accessible (low)

validateOrder() will never return zero status as non-matching signature is handled on the spot.

Proof of Concept

if (signaturesigner != o.signer) condition cannot be met:

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

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

Recommended Mitigation Steps

Consider either removing the separate require and handling the errors downstream:

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

Or removing non-matching signature 0 case as redundant

3. GolomTrader uses hard-coded protocol fee (non-critical)

Protocol fee is now hard coded across the logic, which is error-prone.

Proof of Concept

fillAsk():

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

        // pay fees of 50 basis points to the distributor
        payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));

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

        } else {
            payEther(
                (o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount,
                o.signer
            );
        }
        payEther(p.paymentAmt, p.paymentAddress);

        distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);

_settleBalances():

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

    function _settleBalances(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p
    ) internal {
        uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;

Recommended Mitigation Steps

Consider introducing the corresponding constants to replace 50 and 10000 values

4. p.paymentAmt check is omitted in GolomTrader's fillCriteriaBid (non-critical)

fillCriteriaBid() allows for extra payment to be included by a taker to the criteria based order fill, but this extra value isn't included into total amount check, making it possible to provide less funds than it will be distributed, which will lead to revert on taker's payment calculation (via setting negative number to uint).

Proof of Concept

fillCriteriaBid()'s description states order of ordertype 2 also has a payment param in case the taker wants to send ether to that address on filling the order, while o.totalAmt check doesn't include p.paymentAmt, making it possible to run _settleBalances when there is a deficit:

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

    /// @dev function to fill a signed order of ordertype 2 also has a payment param in case the taker wants
    ///      to send ether to that address on filling the order, Match an criteria order, ensuring that the supplied proof demonstrates inclusion of the tokenId in the associated merkle root, if root is 0 then any token can be used to fill the order
    /// @param o the Order struct to be filled must be orderType 2
    /// @param amount the amount of times the order is to be filled(useful for ERC1155)
    /// @param referrer referrer of the order
    /// @param p any extra payment that the taker of this order wanna send on succesful execution of order
    function fillCriteriaBid(
        Order calldata o,
        uint256 amount,
        uint256 tokenId,
        bytes32[] calldata proof,
        address referrer,
        Payment calldata p
    ) public nonReentrant {
        require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);

While fillCriteriaBid() doesn't check for p.paymentAmt, it's still always paid out in _settleBalances(o, amount, referrer, p):

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

    payEther(p.paymentAmt, p.paymentAddress);

However, _settleBalances() will revert little bit earlier as - p.paymentAmt is included to taker's share calculation, making the resulting amount negative when there is a deficit:

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

        if (o.refererrAmt > 0 && referrer != address(0)) {
            payEther(o.refererrAmt * amount, referrer);
            payEther(
                (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
                    amount -
                    p.paymentAmt,
                msg.sender
            );
        } else {
            payEther(
                (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
                msg.sender
            );
        }

Recommended Mitigation Steps

It's recommended to avoid such low level reverts as they make troubleshooting and programmatic usage of the system difficult.

Consider including extra p.paymentAmt amount to the total amount check:

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

-   require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
+   require(o.totalAmt * amount >= (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt);

5. Redundant comments (non-critical)

No code corresponds to the comment as ETH isn't used, WETH funds are transferred directly thereafter:

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

 // require eth amt is sufficient

Recommended Mitigation Steps

Consider removal of the line

6. Multiple asserts in VoteEscrowCore (non-critical)

Assert will consume all the available gas, providing no additional benefits when being used instead of require, which both returns gas and allows for error message.

Proof of Concept

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

    function _addTokenTo(address _to, uint256 _tokenId) internal {
        // Throws if `_tokenId` is owned by someone
        assert(idToOwner[_tokenId] == address(0));

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

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

...

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

    function totalSupplyAt(uint256 _block) external view returns (uint256) {
        assert(_block <= block.number);

Recommended Mitigation Steps

Using assert in production isn't recommended, consider substituting it with require in all the cases

7. VoteEscrowCore's withdraw updates the internal state with _burn after sending out the funds (non-critical)

VoteEscrowCore's withdraw() sends out the funds before internal state is updated fully, as _burn happens after transfer.

This is dangerous as token even having no transfer flow control now, can be upgraded in the future, while source function nonreentrant modifier alone doesn't fully protect against cross-function reentrancy.

Proof of Concept

_tokenId is burned after value of token is sent out in the end of withdraw():

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

    /// @notice Withdraw all tokens for `_tokenId`
    /// @dev Only possible if the lock has expired
    function withdraw(uint256 _tokenId) external nonreentrant {
        assert(_isApprovedOrOwner(msg.sender, _tokenId));
        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

        LockedBalance memory _locked = locked[_tokenId];
        require(block.timestamp >= _locked.end, "The lock didn't expire");
        uint256 value = uint256(int256(_locked.amount));

        locked[_tokenId] = LockedBalance(0, 0);
        uint256 supply_before = supply;
        supply = supply_before - value;

        // old_locked can have either expired <= timestamp or zero end
        // _locked has only 0 end
        // Both can have >= 0 amount
        _checkpoint(_tokenId, _locked, LockedBalance(0, 0));

        assert(IERC20(token).transfer(msg.sender, value));

        // Burn the NFT
        _burn(_tokenId);

Recommended Mitigation Steps

Consider updating the operation flow as an ultimate way to close the reentrancy surface:

-       assert(IERC20(token).transfer(msg.sender, value));

        // Burn the NFT
        _burn(_tokenId);

+       require(IERC20(token).transfer(msg.sender, value), 'transfer failed');        

8. New operation initiating user facing GolomTrader functions miss emergency lever (non-critical)

If there be any emergency with system contracts, there is no way to temporary stop the operations.

Proof of Concept

The contract doesn't have pausing functionality for the new operation initiation.

fillAsk(), fillBid() and fillCriteriaBid() cannot be temporary paused:

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

    function fillAsk(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p,
        address receiver
    ) public payable nonReentrant {

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

    function fillBid(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p
    ) public nonReentrant {

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

    function fillCriteriaBid(
        Order calldata o,
        uint256 amount,
        uint256 tokenId,
        bytes32[] calldata proof,
        address referrer,
        Payment calldata p
    ) public nonReentrant {

Recommended Mitigation Steps

Consider making all new actions linked user facing functions pausable, for example, by using OpenZeppelin's approach:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol

9. Typos in comments (non-critical)

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

    /// @dev Exeute transfer of a NFT.

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

   // emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining

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

// emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining

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

    /// @param p any extra payment that the taker of this order wanna send on succesful execution of order
    function fillCriteriaBid(

Recommended Mitigation Steps

Consider fixing the spelling

10. Debug comment (non-critical)

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

    //console.log(block.timestamp,epoch,fee);

Recommended Mitigation Steps

Consider removing before release

1. ve.ownerOf(tokenids[tindex]) is called extra time

Proof of Concept

tokenowner is ve.ownerOf(tokenids[0]):

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

    function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {
        require(address(ve) != address(0), ' VE not added yet');

        uint256 reward = 0;
        uint256 rewardEth = 0;
        address tokenowner = ve.ownerOf(tokenids[0]);

So there is no need to repeat ve.ownerOf(tokenids[0]) request, which is expensive, during the first iteration:

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

        // for each tokenid
        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
            require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');

Recommended Mitigation Steps

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

        // for each tokenid
        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
-           require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');
+           if (tindex > 0) require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');

2. addFee performs up to 3 very same total supply requests

totalSupply() call is reasonably expensive, but is done repeatedly with the same result.

Proof of Concept

rewardToken.totalSupply() is called up to 3 times with the same result as no minting is performed:

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

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

        // if 24 hours have passed since last epoch change
        if (block.timestamp > startTime + (epoch) * secsInDay) {
            // this assumes atleast 1 trade is done daily??????
            // logic to decide how much token to emit
            // emission = daily * (1 - (balance of locker/ total supply))  full if 0 locked and 0 if all locked
            // uint256 tokenToEmit = dailyEmission * rewardToken.balanceOf()/
            // emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining
            uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
                rewardToken.totalSupply();
            uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();

Recommended Mitigation Steps

Consider saving the result of the first call to memory variable, reusing it inside if (block.timestamp > startTime + (epoch) * secsInDay) { block as this block will be run in big enough share of addFee() calls.

3. NFT balance and total supply of ve token is requested twice

ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]])) / ve.totalSupplyAt(epochBeginTime[epochs[index]]) is calculated twice for each index with epochs[index] > 0. Both balanceOf and totalSupply are costly operations.

Proof of Concept

multiStakerClaim uses the same share for reward and rewardEth:

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

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

Recommended Mitigation Steps

Consider saving ve.balanceOfAtNFT(tokenids[tindex], epochBeginTime[epochs[index]])) and ve.totalSupplyAt(epochBeginTime[epochs[index]]) to memory and using both variables for reward and rewardEth:

                } else {
                    reward += rewardStaker[epochs[index]] * veBalanceAtIndex / veSupplyAtIndex;
                    rewardEth += epochTotalFee[epochs[index]] * veBalanceAtIndex / veSupplyAtIndex;
                }