Golom contest - Junnon's results

Missing zero-address check

zero-address check is best practice that prevent a transfer to zero-address! Instance :

1. VoteEscrowCore.sol VoreEscrowCore::setVoter() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L868-L871

function setVoter(address _voter) external {
        require(msg.sender == voter);
        voter = _voter;
    }

the function lack of zero-address checking, once voter changed to zero-address voter cannot be retrieved back, and voter is a important role. It also recommended to use 2 step instead to mitigated that voter changed to address we don't want to.

2. VoteEscrowDelegation.sol VoteEscrow::constructor() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L53

        token = _token;

3. RewardDistribution.sol RewardDistribution::constructor() 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;
    }

RewardDistribution::changeTrader() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L285-L288

    function changeTrader(address _trader) external onlyOwner {
        traderEnableDate = block.timestamp + 1 days;
        pendingTrader = _trader;
    }

4. GolomTrader.sol GolomTrader::constructor() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L92

5. GolomToken.sol GolomToken::setMinter() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L58-L61

Mitigation: check for address(0)

require(_minter != address(0),"zero-address")

Missing emit event in important state change

some state change doesn't emit event, emiting an event important for off chain monitoring.

Instance: GolomToken.sol

1. GolomToken::executeSetMinter() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L65

GolomTrader.sol

1. GolomTrader::executeSetDistributor() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L454-L457

RewardDistributor.sol

1. RewardDistributor::executeChangeTrader() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L291-L294
2. RewardDistributor::executeAddVoteEscrow() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L308-L311

VoteEscrowCore.sol

1. VoteEscrowCore::_checkpoint() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L690-L825

VoteEscrowDelegation.sol

1. VoteEscrow::removeDelegation() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L210-L216
2. VoteEscrow::changeMinVotingPower() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L260-L262

Calculation in GolomTrader.sol can be inaccurate.

this calculation occure often in GolomTrader.sol:

(o.totalAmt * 50) / 10000

this calculation can be inaccurate if o.totalAmt is low enought, it's happens because uint have no decimals and will rounding down that number, that make the distributor doesn't take a accurate reward . since it need totalAmt to be low the impact not too big so i report it in QA Report.

Mitigation: totalAmt can be multiplied by much larger number and when it need to be use it can be devide by that number example:

       ///original
        require(
            o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,
            'amt not matching'
        );

        ///mitigated
         uint largeNum = 10000; //can be much larger and defined as constant 
         require(
            o.totalAmt * largeNum >= 
                        o.exchange.paymentAmt * largeNum + 
                        o.prePayment.paymentAmt * largeNum+ 
                        o.refererrAmt * largeNum + 
                        (o.totalAmt * largeNum * 50) / 10000,
            'amt not matching'
        );

State that only declared in constructor can be marked as immutable

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

State can be marked as constant

Instance: RewardDistributor.sol RewardDistributor::startTime (can be declared outside constructor since it a constant value) https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L84

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

Function with public mark that never called in the contract can be marked as external instead

Instance: RewardDistributor.sol :

1. RewardDistributor::addFee() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L98
2. RewardDistributor::traderClaim() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L141
3. RewardDistributor::exchangeClaim() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L155
4. RewardDistributor::multiStakerClaim() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L172
5. RewardDistributor::stakerRewards() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L215
6. RewardDistributor::traderRewards() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L254
7. RewardDistributor::exchangeRewards() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L269

GolomTrader.sol :

1. GolomTrader::fillAsk(); https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L203
2. GolomTrader::fillBid(); https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L279
3. GolomTrader::cancelOrder(); https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L312
4. GolomTrader::fillCriteriaBid() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L334

VoteEnscrowDelegation.sol

1. VoteEscrow::getVotes() https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L168

hardhat/console.sol can be removed from import since it isn't needed except in testing

hardhat/console.sol can be very costly to deploy it better to remove it since it isn't needed in actual implementation.

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

Variable state thats non-constant that have initial value 0 can be left by default instead of passing it with 0, and avoid using array.length for loop.

because the instance almost same i decide to report it in one tittle:

1. uint variable in solidity have default value 0, it isn't needed to pass it with 0 value (which will cost gas).
2. array.length cost gas for every loop, try to avoid it, pass it to a local variable before loop for one time operation.

example :

uint length = array.length;
for (uint i, i < length, ++i) {}

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

50:    uint256 public MIN_VOTING_POWER_REQUIRED = 0;
171:         for (uint256 index = 0; index < delegated.length; index++) {
181:         for (uint256 index = 0; index < delegatednft.length; index++) {

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

745:             for (uint256 i = 0; i < 255; ++i) {
1044:         for (uint256 i = 0; i < 128; ++i) {

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

143:         for (uint256 index = 0; index < epochs.length; index++) {
157:         for (uint256 index = 0; index < epochs.length; index++) {
180:         for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
183:             for (uint256 index = 0; index < epochs.length; index++) {
226:         for (uint256 index = 0; index < epoch; index++) {
258:         for (uint256 index = 0; index < epoch; index++) {
273:         for (uint256 index = 0; index < epoch; index++) {

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

415:        for (uint256 i = 0; i < proof.length; i++) {

Unused interface can be removed instead

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