Golom contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

All ETH and GOLOM that accumulate in this contract for stakers would be irretrievable

Proof of Concept

ve = VE(pendingVoteEscrow);

L300 sets ve to the wrong variable. It sets VE = pendingVotingEscrow which is never set elsewhere and is address(0). This means that the else statement is never triggered and pendingVoteEscrow is always address(0), making it impossible to ever set VE to anything other than address(0).

Tools Used

Recommended Mitigation Steps

Change L300 to:

ve = VE(_voteEscrow);

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L94-L109

Vulnerability details

Impact

Delegation is completely broken

Proof of Concept

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

In the above code, for the first checkpoint (numCheckpoints[toTokenId] = 0) L85 will be executed passing in nCheckpoints = 0 to _writeCheckpoint.

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

This causes _writeCheckpoint to underflow at L101 (shown above) causing a revert. Since no token can ever be checkpoint for the first time, the delegation system is completely broken

Tools Used

Recommended Mitigation Steps

Create a nested if statement and move L101 up inside as shown below:

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

    if (nCheckpoints > 0){
        Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
        if (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/core/GolomTrader.sol#L375-L403

Vulnerability details

Impact

Sales of amount > 1 give too little ETH and will always revert if more than 200 are sold at one time

Proof of Concept

    uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;

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

protocolFee already accounts for the amount being sold when calculated in L381. When calling payEther in L390 and L397, protocolfee is multiplied by amount a second time. This results in msg.sender not receiving the proper amount of ETH and loss of user funds. If amount >= 200 then protocolfee >= o.totalAmt which will cause an underflow error. This affects fillBid and fillCriteriaBid because both call _settleBalances for ETH transfers.

Tools Used

Recommended Mitigation Steps

Move protocolfee out of the parentheses as shown below:

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

Lines of code

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

Vulnerability details

Submitting as high risk because even though there is no direct loss of funds, the large amount of votes could be used to hijack governance which could lead to loss of funds.

Impact

Unfair number of votes

Proof of Concept

delegate has no check that tokenID has not already been delegated. This allows a user to multiply their votes by delegating repeatedly. In L80 tokenID gets pushed onto the end of delegatedTokenID each time delegate is called. When getVotes is called, tokenID's votes will get counted each time it is in the array, inflating the voting power associated with it.

Tools Used

Recommended Mitigation Steps

I recommend adding the following code to the start of delegate:

currentDelegate = delegates[tokenId]

if (currentDelegate != address(0)) {
    require(currentDelegate != toTokenId)
    removeDelegation(tokenId)
}

Lines of code

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

Vulnerability details

Impact

Protocol fees will no longer be distributed after GOLOM inflation ends

Proof of Concept

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

addFee contains the lines above that short circuit the function when there are not more GOLOM rewards to mint. This works to prevent additional GOLOM rewards from being minted and distributed but it also stops tracking the amount of ETH fees and epoch. This means that after GOLOM rewards run out, distribution of WETH rewards will also stop because they are no longer properly tracked

Tools Used

Recommended Mitigation Steps

Move all epoch and protocol fee logic to before the short circuit so that it still functions after GOLOM inflation ends

Lines of code

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

Vulnerability details

Submitting as high risk because even though there is no direct loss of funds, the misrepresentation of votes could be detrimental for governance which by extent could lead to loss of funds.

Impact

Delegation isn't actually removed in a majority of cases

Proof of Concept

The function always removes tokenID from it's own checkpoint.delegatedTokenIds. This would only effectively remove delegation from the token if it was delegated to itself. Instead it should remove tokenId from whatever token it is currently delegated to.

Tools Used

Recommended Mitigation Steps

Rewrite removeDelegation as follows, removing the delegation from where it is currently delegated to:

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

        if (currentDelegate != address(0)) {
            uint256 nCheckpoints = numCheckpoints[currentDelegate];
            Checkpoint storage checkpoint = checkpoints[currentDelegate][nCheckpoints - 1];
            removeElement(checkpoint.delegatedTokenIds, tokenId);
            _writeCheckpoint(currentDelegate, nCheckpoints, checkpoint.delegatedTokenIds);
       }
    }

Lines of code

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

Vulnerability details

Impact

The use of the deprecated transfer() function for an address payable will cause transactions to fail in a large number of scenarios - see https://github.com/code-423n4/2022-05-bunker-findings/issues/116

Proof of Concept

Tools Used

Recommended Mitigation Steps

Use call() instead of transfer()

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

[L-02] Use safeTransferFrom instead of transferFrom for ERC721 transfers Description Always best practice to use safeTransferFrom instead of transferFrom to protect users from input mistakes. Avoids a large number of other potential problems and should be the default anytime tokens are transferred to an arbitrary address.

Lines of Code https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L9 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

Lines of code

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

Vulnerability details

Impact

Order unable to be cancelled by cancelOrder

Proof of Concept

    filled[hashStruct] = o.tokenAmt + 1;

cancelOrder will overflow in the line shown above if o.tokenAmt is type(uint256).max causing the transaction to always revert for that order.

Tools Used

Recommended Mitigation Steps

I don't see any reason why 1 should be added to o.tokenAmt, change to:

    filled[hashStruct] = o.tokenAmt;

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L14-L73

Vulnerability details

Impact

Potential downtime in GolomTrader

Proof of Concept

GolomToken.sol doesn't have a function to mint the treasury tokens as specified in the docs (https://docs.golom.io/tokenomics-and-airdrop). In order for these tokens to be minted, the minter would have to be changed via setMinter() and executeSetMinter() to a contract that can mint the treasury tokens. Because of the 24 hour timelock, this would lead to downtime for GolomTrader.sol if trading has already begun. This is because GolomTrader.sol calls RewardDistributor.sol#addFees each time there is a filled order. When the epoch changes, RewardDistributor.sol will try to call the mint function in GolomToken.sol. Because of the timelock, there will be at least a 24 hours period where RewardDistributor.sol is not the minter and doesn't have the permission to mint. This means that during that period all trades will revert.

Tools Used

Recommended Mitigation Steps

Add a function to GolomToken.sol to mint the treasury tokens similar to the mintAirdrop() and mintGenesisReward() functions.

Lines of code

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

Vulnerability details

Impact

Burn NFTs remained delegated causing bloat and wasting gas

Proof of Concept

VoteEscrowDelegation.sol doesn't change the withdraw or _burn functions inherited from VoteEscrowCore.sol. These functions are ignorant of the delegation system and don't properly remove the delegation when burning an NFT. The votes for the burned NFT will be removed but the reference will still be stored in the delegation list where it was last delegated. This creates a few issues. 1) It adds bloat to both getVotes and getPriorVotes because it adds a useless element that must be looped through. 2) The max number of users that can delegate to another NFT is 500 and the burned NFT takes up one of those spots reducing the number of real users that can delegate. 3) Adds gas cost when calling removeDelegation which adds gas cost to _transferFrom because removeElement has to cycle through a larger number of elements.

Tools Used

Recommended Mitigation Steps

Override _burn in VoteEscrowDelegation and add this.removeDelegation(_tokenId), similar to how it was done in _transferFrom

Lines of code

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

Vulnerability details

Impact

Rewards owed burned NFT are permanently locked

Proof of Concept

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

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

After an NFT is burned, owner of token is set to address(0).

    rewardToken.transfer(tokenowner, reward);

This causes issues in multiStakerClaim L208. GOLOM uses OZ's implementation of ERC20 which doesn't allow tokens to be sent to address(0). Because the "owner" of the burned NFT is address(0) multiStakerClaim will always revert when called for a burned NFT trapping rewards in contract forever.

Tools Used

Recommended Mitigation Steps

Implement a clawback clause inside the multiStakerClaim function. If the token is burned (i.e. owned by address(0)) the rewards should be transferred to different address. These rewards could be claimed to the treasury or burned, etc.

    if (tokenowner == address(0){
        rewardToken.transfer(treasury, reward);
        weth.transfer(treasury, rewardEth);
    }

Lines of code

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

Vulnerability details

Impact

Loss of delegation to specific NFTs

Proof of Concept

    require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

In L99 delegation is blocked if the NFT being delegated to currently has 500 delegatees. This limitation prevents OOG errors that could originate from removeElement if the list of delegatees grows too long. By implementing a cap it allows a malicious user to DOS legitimate users by creating 500 NFTs and delegating to another NFT to keep it from being able to be delegated to by legitimate users.

Tools Used

Recommended Mitigation Steps

This can be mitigated by implementing OZ's enumerableSet library - https://docs.openzeppelin.com/contracts/3.x/api/utils. Gas cost for removal of an element is fixed for any length list allowing the removal of the 500 delegatee cap, removing this DOS vector. I strongly recommend implementing this even if only to save the gas costs of users who wish to delegate to NFTs with a high number of delegatees.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L233-L256

Vulnerability details

Submitting as high risk because it breaks a fundamental operation (transferring) for a large number of tokens

Impact

NFTs that don't have a checkpoint can't be transferred

Proof of Concept

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

    uint256 nCheckpoints = numCheckpoints[tokenId];
    Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];

L242 of transferFrom() calls removeDelegation() with the tokenId of the token being transferred. For tokens that don't have any checkpoints, L212 will return 0. This was cause an underflow error and revert in L213.

Tools Used

Recommended Mitigation Steps

Make removeDelegation simply return if nCheckpoints = 0

QA Report

Non-Critical Findings

[NC-01] Usage of 1e18 vs 10**18

Description

Both 1e18 vs 1018 are equivalent and both work fine but 1018 is more typical. Regardless of which you want to use, pick one and stick to it.

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L44 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L52 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L48 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L100

Mitigation

Pick either 1e18 vs 10**18 and use it consistently across all contracts

[NC-02] Calculate once and reuse protocolfee in GolomTrader.sol#fillAsk

Description

(o.totalAmt * 50) / 10000 is calculated repeatedly in fillAsk. Having the same repeated calculation can lead to errors if changed later and some instances are missed. Better to do it once and reuse the results.

Lines of Code

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

Mitigation

(o.totalAmt * 50) / 10000 should just be calculated once, stored and reused like it is in _settleBalance

[NC-03] GolomTrader.sol#fillAsk fillBid and fillCriteriaBid should be updated so NFT transfer code matches

Description

In fillCriteriaBid and fillBid o.collection is first stored as nftcontract then the transfer is called on nftcontract. In fillAsk it is just called directly. Both methods are valid but they should match in format for consistency and readability.

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L234-L239 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L298-L305 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L298-L305

Mitigation

Change fillAsk to use the same format as fillCriteriaBid and fillBid

[NC-04] rewardLP and claimedUpto are declared in RewardDistributor.sol but never used

Description

No reason to declare variables then never use them.

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L63 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L66

Mitigation

Remove variables

[NC-05] RewardDistributor.sol#stakerRewards returns an array of 0's and 1's

Description

stakerRewards returns an array of 0's and 1's but multiStakerClaim requires an array with the numbers of the epochs that need to be claimed. This means that the output of stakersRewards must be interpreted to use with multiStakerClaim

Lines of Code

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

Mitigation

stakerRewards should be changed to return the numbers of the epochs that need to be claimed. Remove L227 and add the following after L228

unclaimedepochs.push(index);

[NC-06] RewardDistributor.sol#traderRewards and exchangeRewards don't return the epochs that need to be claimed

Description

traderRewards and exchangeRewards don't return the epochs that need to be claimed, they only return the amount that can be claim which has limited usage. traderClaim and exchangeClaim both require an array of epochs to claim. It would be useful if traderRewards and exchangeRewards were changed to return the epoch numbers that need to be claimed.

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L254-L265 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L269-L280

Mitigation

Example of how to change exchangeRewards:

function exchangeRewards(address addr) public view returns (
        uint256,
        uint256[]
    ){
    uint256 reward = 0;
    uint256[] memory unclaimedepochs = new uint256[]
    for (uint256 index = 0; index < epoch; index++) {
        uint256 epochReward = (rewardExchange[index] * feesExchange[addr][index]) / epochTotalFee[index];
        if (epochReward > 0) {
            unclaimedepochs.push(index)
            reward = reward + epochReward
        }
    }
    return (reward, unclaimedepochs);
}

Low Risk Findings

[L-01] GolomToken.sol has no reason to implement ERC20Votes

Description

GolomToken.sol implements ERC20Votes but never uses any of the features associated with it. This adds needless complexity and increases gas costs to deploy and use the tokens due to the checkpoint system implemented in ERC20Votes

Lines of Code

N/A

Mitigation

Remove ERC20Votes. Alternatively inherit ERC20Permit if the use of permits is desired or simply inherit ERC20

[L-02] Use safeTransferFrom instead of transferFrom for ERC721 transfers

Description

Always best practice to use safeTransferFrom instead of transferFrom to protect users from input mistakes. Avoids a large number of other potential problems and should be the default anytime tokens are transferred to an arbitrary address.

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L9 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

Mitigation

Change transferFrom to safeTransferFrom

[L-03] Require statement in GolomTrader.sol#fillCriteriaBid should be updated to match fillBid

Description

fillCriteriaBid doesn't check for p.paymentAmt like fillBid does. Not having it doesn't present any vulnerabilities but it helps to revert an invalid transaction earlier and save user gas.

Lines of Code

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

Mitigation

Change L342 to match L285-287

[L-04] ERC20 interface should be called WETH instead

Description

Interface is called ERC20 but it's not actually ERC20 because ERC20 doesn't implement a withdraw function. This is only true of WETH. To avoid any confusion, the interface should be called WETH not ERC20

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L26 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L11

Mitigation

Change interface name from ERC20 to WETH

[L-05] startTime is set to a time in the past in RewardDistributor.sol#contructor

Description

The startTime in the constructor has already passed and will cause a large amount of minting as soon the contract is deployed and users begin trading. This is becuase epochs are calculated as a fixed value from startTime in L106. This will cause the epoch to rapidly snap forward one epoch each trade until it catches up to realtime. This will cause many days of emissions in the first day that it's launched.

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L84

Mitigation

Update time before launching contract

[L-06] Inconsistent permissioning for VoteEscrowDelegation.sol#delegate and removeDelegation

Description

delegate and removeDelegation require that only the owner can call the function but every other function in VoteEscrowDelegation and VoteEscrowCore uses both owner and approved

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L72 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L211

Mitigation

Change to:

require(_isApprovedOrOwner(msg.sender, _tokenId));

[L-07] Use of deprecated assert

Description

Assert consumes all of user's remaining gas. There isn't any reason for this and require should be used instead.

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L493 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L506 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L519 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L666 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L679 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L861 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L977 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L981 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L991 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1007 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1023 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1110 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1206

Mitigation

Replace assert with require

[L-08] Use of floating pragma

Description

Contracts should be deployed with the same compiler version and flags that they have been designed and tested. Different compiler versions/flags may introduce bugs that affect the contract negatively. Locking the pragma helps avoid this.

Lines of Code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L2

Mitigation

Match the rest of the code base and lock to 0.8.11:

pragma solidity 0.8.11;