Golom contest - cccz's results

Lines of code

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

Vulnerability details

Impact

The delegate function of the VoteEscrow contract allows users to reuse votes to vote for multiple tokenIDs Consider the following scenario, where user A has 100 votes. User A calls the delegate function to vote for tokenID 2 and 3 respectively Calling the getVotes function for tokenID 2 and 3 will return 100 That is, user A uses 100 votes to get the effect of 200 votes

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Remove vote on previous delegates[tokenId] in delegate function

Lines of code

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

Vulnerability details

Impact

In the removeDelegation function of the VoteEscrow contract, the user is only allowed to remove their own votes against themselves, which makes it impossible for the user to remove their votes against other users.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

change

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

to

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

Lines of code

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

Vulnerability details

Impact

In GolomTrader contract, payEther function calls native payable.transfer. This is unsafe as transfer has hard coded gas budget and can fail when the user is a smart contract.

Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

Lines of code

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

Vulnerability details

Impact

When using the transferFrom function of an ERC721 contract to send an NFT, if the receiving address is a smart contract and does not support ERC721, the NFT can be frozen in the contract.

Proof of Concept

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L236-L237 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L301-L302 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L361-L362

Tools Used

None

Recommended Mitigation Steps

Use the ERC721 contract's safeTransferFrom function to send NFTs

Lines of code

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

Vulnerability details

Impact

According to https://eips.ethereum.org/EIPS/eip-721#specification, the safeTransferFrom function needs to check that the return value of onERC721Received is bytes4(keccak256("onERC721Received(address,address,uint,bytes)"))

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

    function safeTransferFrom(
        address _from,
        address _to,
        uint256 _tokenId,
        bytes memory _data
    ) public {
        _transferFrom(_from, _to, _tokenId, msg.sender);

        if (_isContract(_to)) {
            // Throws if transfer destination is a contract which does not implement 'onERC721Received'
            try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4 retval) {
+                return retval == IERC721Receiver.onERC721Received.selector;
            } catch (
                bytes memory reason

Lines of code

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

Vulnerability details

Impact

When MIN_VOTING_POWER_REQUIRED is changed, tokenIDs with votes lower than MIN_VOTING_POWER_REQUIRED will not be able to vote through the delegate function, but previous votes will not be affected. Since MIN_VOTING_POWER_REQUIRED is mainly used to reduce the influence of spam users, changing this value should affect previous votes.

Proof of Concept

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L168-L194 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L260-L262 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L73-L74

Tools Used

None

Recommended Mitigation Steps

In the getPriorVotes and getVotes functions, when the balance corresponding to tokenId is less than MIN_VOTING_POWER_REQUIRED, the value of votes will not be increased

    function getVotes(uint256 tokenId) external view returns (uint256) {
        uint256[] memory delegated = _getCurrentDelegated(tokenId);
        uint256 votes = 0;
        for (uint256 index = 0; index < delegated.length; index++) {
+         if(this.balanceOfNFT(delegated[index]) >= MIN_VOTING_POWER_REQUIRED){
            votes = votes + this.balanceOfNFT(delegated[index]);
+       }
        }
        return votes;
    }


    /**
     * @notice Determine the prior number of votes for an account as of a block number
     * @dev Block number must be a finalized block or else this function will revert to prevent misinformation.
     * @param tokenId The address of the account to check
     * @param blockNumber The block number to get the vote balance at
     * @return The number of votes the account had as of the given block
     */
    function getPriorVotes(uint256 tokenId, uint256 blockNumber) public view returns (uint256) {
        require(blockNumber < block.number, 'VEDelegation: not yet determined');
        uint256[] memory delegatednft = _getPriorDelegated(tokenId, blockNumber);
        uint256 votes = 0;
        for (uint256 index = 0; index < delegatednft.length; index++) {
+         if(this.balanceOfAtNFT(delegatednft[index], blockNumber) >= MIN_VOTING_POWER_REQUIRED){
            votes = votes + this.balanceOfAtNFT(delegatednft[index], blockNumber);
+         }
        }
        return votes;
    }

Lines of code

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

Vulnerability details

Impact

The validateOrder function of GolomTrader calls the Solidity ecrecover function directly to verify the given signatures. The return value of ecrecover may be 0, which means the signature is invalid, but the check can be bypassed when signer is 0.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Use the recover function from OpenZeppelin's ECDSA library for signature verification.

Lines of code

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

Vulnerability details

Impact

In the fillAsk function of the GolomTrader contract, when msg.value > o.totalAmt * amount + p.paymentAmt, the excess ETH is not refunded

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Refund the excess ETH to the caller