Golom contest - GimelSec's results

Lines of code

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

Vulnerability details

Impact

RewardDistributor.addVoteEscrow has a serious mistake. The contract owner cannot add any vote escrow contract.

Proof of Concept

Initially, address(ve) and address(pendingVoteEscrow) are address(0). So the if condition is always true. Thus, the owner cannot add any vote escrow contract. https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L300

    function addVoteEscrow(address _voteEscrow) external onlyOwner {
        if (address(ve) == address(0)) {
            ve = VE(pendingVoteEscrow);
        } else {
            voteEscrowEnableDate = block.timestamp + 1 days;
            pendingVoteEscrow = _voteEscrow;
        }
    }

Tools Used

None

Recommended Mitigation Steps

Fix addVoteEscrow

    function addVoteEscrow(address _voteEscrow) external onlyOwner {
        if (address(ve) == address(0)) {
            ve = VE(_voteEscrow);
        } else {
            voteEscrowEnableDate = block.timestamp + 1 days;
            pendingVoteEscrow = _voteEscrow;
        }
    }

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L101 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L82-L86

Vulnerability details

Impact

When a user call VoteEscrowDelegation.delegate to make a delegation, it calls VoteEscrowDelegation._writeCheckpoint to update the checkpoint of toTokenId. However, if nCheckpoints is 0, _writeCheckpoint always reverts. What’s worse, nCheckpoints would be zero before any delegation has been made. In conclusion, users cannot make any delegation.

Proof of Concept

When a user call VoteEscrowDelegation.delegate to make a delegation, it calls VoteEscrowDelegation._writeCheckpoint to update the checkpoint of toTokenId. https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L82-L86

    function delegate(uint256 tokenId, uint256 toTokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');

        delegates[tokenId] = toTokenId;
        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);
        }

        emit DelegateChanged(tokenId, toTokenId, msg.sender);
    }

if nCheckpoints is 0, _writeCheckpoint always reverts. Because checkpoints[toTokenId][nCheckpoints - 1] will trigger underflow in Solidity 0.8.11 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L101

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

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

Tools Used

None

Recommended Mitigation Steps

Fix _writeCheckpoint

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

   

        if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
            Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
            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/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L71

Vulnerability details

Impact

While delegating voting power to a new delegatee, if any current delegatee exists, it should be cleared to ensure no NFT can be used to vote twice. The current implementation does not do this and allows attackers to delegate multiple times and multiply their voting power.

Proof of Concept

Current implementation of VoteEscrowDelegation.delegate does not check if any delegatee exists and clear it before assigning a new delegatee.

    function delegate(uint256 tokenId, uint256 toTokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');

        delegates[tokenId] = toTokenId;
        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);
        }

        emit DelegateChanged(tokenId, toTokenId, msg.sender);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Check and clear any existing delegations

    function delegate(uint256 tokenId, uint256 toTokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');

        if (delegates[tokenId] != 0) {
            removeDelegation(tokenId);
        }
        delegates[tokenId] = toTokenId;
        uint256 nCheckpoints = numCheckpoints[toTokenId];
        …
    }

Lines of code

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

Vulnerability details

Impact

RewardDistributor.sol addFee stops all operations after minting limit of reward token in met, future ETH transferred to RewardDistributor by GolomTrader will be stuck.

Proof of Concept

When addFee rewardToken.totalSupply() > 1000000000 * 10**18, it will return directly: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L100-L103

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

Then weth.deposit will not be called: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L124-L130

            if (previousEpochFee > 0) {
                if (epoch == 1){
                    epochTotalFee[0] =  address(this).balance; // staking and trading rewards start at epoch 1, for epoch 0 all contract ETH balance is converted to staker rewards rewards.
                    weth.deposit{value: address(this).balance}();  
                }else{
                    weth.deposit{value: previousEpochFee}();
                }

It weth.deposit doesn't be called, any ETH in RewardDistributor (which is transferred by addFee, or accidentally send ETH in receive()) will be stuck.

Tools Used

None

Recommended Mitigation Steps

It should call weth.deposit first before returning.

Lines of code

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

Vulnerability details

Impact

VoteEscrowDelegation.removeDelegation attempts to update checkpoints for the wrong NFT, and fails to remove delegation correctly. This will result in incorrect/unexpected delegation relationships between tokens after transfer.

Proof of Concept

Delegation in VoteEscrowDelegation is implemented by maintaining two structures. The first is delegates, which keeps track of delegator NFT->delegatee NFT mappings. The second is checkpoints.delegatedTokenIds, which is responsible for recording delegatee NFT->delegator NFT list.

    /// @notice Delegate of the specific token
    mapping(uint256 => uint256) public delegates;
    …
    /// @notice A checkpoint for marking number of votes from a given block
    struct Checkpoint {
        uint256 fromBlock;
        uint256[] delegatedTokenIds;
    }

    /// @notice A record of votes checkpoints for each tokenId, by index
    mapping(uint256 => mapping(uint256 => Checkpoint)) public checkpoints;

Correct way to remove delegation of NFT X is to clear delegates[X] and remove X from checkpoint[delegates[X]].delegatedTokenIDs. However, the current way removeDelegation does this is by removing X from checkpoint[X].delegatedTokenIDs, which does not make sense.

    function removeDelegation(uint256 tokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        uint256 nCheckpoints = numCheckpoints[tokenId];
        Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];
        removeElement(checkpoint.delegatedTokenIds, tokenId);
        _writeCheckpoint(tokenId, nCheckpoints, checkpoint.delegatedTokenIds);
    }

This incorrect implementation will result in delegations to never be removed from delegatee.

Tools Used

Manual audit

Recommended Mitigation Steps

Do bookkeeping correctly as shown in snippet below

    function removeDelegation(uint256 tokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        uint256 delegatee = delegates[tokenId];
        if (delegatee != 0) {
            delegates[tokenId] = 0;
            uint256 nCheckpoints = numCheckpoints[delegatee];
            Checkpoint storage checkpoint = checkpoints[delegatee][nCheckpoints - 1];
            removeElement(checkpoint.delegatedTokenIds, tokenId);
            _writeCheckpoint(delegatee, nCheckpoints, checkpoint.delegatedTokenIds);
        }
    }

Lines of code

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

Vulnerability details

Impact

Transfer ETH by using transfer() may cause this transaction to fail. In EIP-1884:

In many cases, a recipient of ether from a CALL will want to issue a LOG. The LOG operation costs 375 plus 375 per topic. If the LOG also wants to do an SLOAD, this change may make some such transfers fail.

Proof of Concept

https://github.com/code-423n4/2022-07-golom/blob/main/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
        }
    }

Tools Used

None

Recommended Mitigation Steps

Use call{value: amount}() instead of transfer:

(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Address: unable to send value, recipient may have reverted");

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

Note that call may suffer from reentrancy attack, it should follow Checks-Effects-Interactions.

Lines of code

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

Vulnerability details

Impact

nftcontract.transferfrom doesn't check whether users will handle ERC721 received. If a user is a contract but doesn’t handle ERC721 received, the ERC721 token will be frozen when receiving ERC721 tokens. Thus, using nftcontract.safetransferfrom instead of nftcontract.transferfrom for ERC721 can avoid such problems.

Proof of Concept

It doesn’t check whether the receiver has implemented the onERC721Received function. If a user doesn't handle ERC721 received, the ERC721 token will be frozen. https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L301

    function fillBid(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p
    ) public nonReentrant {
        …
        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);
        } else {
            ERC1155 nftcontract = ERC1155(o.collection);
            nftcontract.safeTransferFrom(msg.sender, o.signer, o.tokenId, amount, '');
        }
        emit OrderFilled(msg.sender, o.signer, 1, hashStruct, o.totalAmt * amount);
        _settleBalances(o, amount, referrer, p);
    }

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

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

        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, tokenId);
        } else {
            ERC1155 nftcontract = ERC1155(o.collection);
            nftcontract.safeTransferFrom(msg.sender, o.signer, tokenId, amount, '');
        }
        emit OrderFilled(msg.sender, o.signer, 2, hashStruct, o.totalAmt * amount);
        _settleBalances(o, amount, referrer, p);
    }

Tools Used

None

Recommended Mitigation Steps

Use safeTransferFrom rather than transferFrom when transferring ERC721 tokens to users.

Lines of code

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

Vulnerability details

Impact

Users are unable to call cancelOrder successfully to cancel order if users set o.tokenAmt to uint256.max.

Proof of Concept

    function cancelOrder(Order calldata o) public nonReentrant {
        require(o.signer == msg.sender);
        (, bytes32 hashStruct, ) = validateOrder(o);
        filled[hashStruct] = o.tokenAmt + 1;
        emit OrderCancelled(hashStruct);
    }

The cancelOrder uses o.tokenAmt + 1 to cancel orders, it may cause integer overflow.

Tools Used

None

Recommended Mitigation Steps

Use o.tokenAmt rather than o.tokenAmt + 1:

    function cancelOrder(Order calldata o) public nonReentrant {
        require(o.signer == msg.sender);
        (, bytes32 hashStruct, ) = validateOrder(o);
        filled[hashStruct] = o.tokenAmt;
        emit OrderCancelled(hashStruct);
    }

Lines of code

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

Vulnerability details

Impact

GolomTrader has a timelock on setting a reward distributor. However, the timelock can be bypassed by a tricky method. And RewardDistributor has the same problem.

Proof of Concept

The timelock on setting a reward distributor can be bypassed in the following steps:

• The owner can set a reward distributor immediately the first time since address(distributor) == address(0) is true. https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L444-L451

    function setDistributor(address _distributor) external onlyOwner {
        if (address(distributor) == address(0)) {
            distributor = Distributor(_distributor);
        } else {
            pendingDistributor = _distributor;
            distributorEnableDate = block.timestamp + 1 days;
        }
    }

• Normally, if the owner wants to set a new distributor, it should call setDistributor again. Then the timelock will be set.
• But the owner can first call executeSetDistributor to bypass the timelock. Since pendingDistributor hasn’t been set, distributor will be address(0) again after calling executeSetDistributor. (And distributorEnableDate <= block.timestamp is true, because distributorEnableDate has not been set.) https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L454-L457

    function executeSetDistributor() external onlyOwner {
        require(distributorEnableDate <= block.timestamp, 'not allowed');
        distributor = Distributor(pendingDistributor);
    }

• When distributor is address(0), the owner can immediately set a new distributor. https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L444-L451

    function setDistributor(address _distributor) external onlyOwner {
        if (address(distributor) == address(0)) {
            distributor = Distributor(_distributor);
        } else {
            pendingDistributor = _distributor;
            distributorEnableDate = block.timestamp + 1 days;
        }
    }

RewardDistributor has the same issue when setting a vote escrow contract.

Tools Used

None

Recommended Mitigation Steps

GolomToken has the correct implementation of timelock mechanism. GolomTrader and RewardDistributor can follow its implementation.

Lines of code

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

Vulnerability details

Impact

VoteEscrowDelegation._transferFrom should be successfully executed if msg.sender is the current owner, an authorized operator, or the approved address. removeDelegation is called in _transferFrom. removeDelegation only accepts the token owner. Thus, _transferFrom can only be executed by the token owner.

Proof of Concept

removeDelegation is called in _transferFrom https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L242

    function _transferFrom(
        address _from,
        address _to,
        uint256 _tokenId,
        address _sender
    ) internal override {
        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

        // remove the delegation
        this.removeDelegation(_tokenId);

        // Check requirements
        require(_isApprovedOrOwner(_sender, _tokenId));
        …
    }

However, removeDelegation only accept the token owner https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L211

    function removeDelegation(uint256 tokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        uint256 nCheckpoints = numCheckpoints[tokenId];
        Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];
        removeElement(checkpoint.delegatedTokenIds, tokenId);
        _writeCheckpoint(tokenId, nCheckpoints, checkpoint.delegatedTokenIds);
    }

Tools Used

None

Recommended Mitigation Steps

Fix the permission control in removeDelegation

Lines of code

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

Vulnerability details

Impact

If a user sends more ETH than the user has to, the contract just accepts it. The user will lose more ETH accidentally.

Proof of Concept

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

        // attached ETH value should be greater than total value of one NFT * total number of NFTs + any extra payment to be given
        require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');

Tools Used

None

Recommended Mitigation Steps

Use == rather than >=:

        require(msg.value == o.totalAmt * amount + p.paymentAmt, 'mgmtm');

Or refund the money.

Lines of code

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

Vulnerability details

Impact

The protocol receives ETH by defining receive() and fallback() functions. Users will lose ETH when he/she accidentally sends ETH to the protocol.

Proof of Concept

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

    fallback() external payable {}

    receive() external payable {}

Tools Used

None

Recommended Mitigation Steps

  receive() external payable {
    require(msg.sender == address(WETH));
  }