Golom contest - teddav's results

Lines of code

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

Vulnerability details

Impact

In RewardDistributor contract, the ve contract can never be set. It’s supposed to be set in addVoteEscrow() (and then confirmed in executeAddVoteEscrow()) But the line if (address(ve) == address(0)) will always be true because the ve variable doesn’t have any other setter. And since pendingVoteEscrow is always 0, it will keep setting ve to address(0).

Recommended Mitigation Steps

Fix: The first call should automatically set the address passed as parameter. Then next calls will implement the timelock Rewrite the function addVoteEscrow() as:

    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/core/GolomTrader.sol#L151

Vulnerability details

Impact

Asks or bids could fail if one of the addresses involved is a contract and cannot receive ETH The function payEther is used in all functions: fillAsk(), fillBid() and fillCriteriaBid() (_settleBalances()). This function just tries to call transfer() to transfer ETH to the receiver. If the receiver is a contract and doesn’t have a receive() payable function, then the .transfer() will revert and the bid or ask will not be able to be filled.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Fix: Add a fallback in payEther() to use WETH if ETH transfer fails

(success, ) = payAddress.call{ value: payAmt }(“”);
if (!success) {
	WETH.deposit{ value: payAmt }(…):
	require(WETH.transfer(payAddress, payAmt));
}

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

Permit signature replay across forks Details: GolomTrader.sol defines chainId at contract deployment without reconstructing it for every signature. However, as stated in the security considerations section of EIP2612, "If the DOMAIN_SEPARATOR contains the chainId and is defined at contract deployment instead of reconstructed for every signature, there is a risk of possible replay attacks between chains in the event of a future chain split.” ****

Impact: Low (depends on the hard forking of a well-established blockchain)

Mitigation: Consider caching the chainId as is done in OpenZeppelin contracts.

Lines of code

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

Vulnerability details

Impact

validateOrder() is vulnerable to signature forgery because it doesn’t check if the result of ecrecover is address(0)

ecrecover returns address(0) if the signature is invalid. Here, the only check made is require(signaturesigner == o.signer, 'invalid signature') Since the Order data is controlled by the user, we can just pass o.signer = address(0). So this check is passed. The rest of the data for the Order can be filled as we desire, so we can transfer to ourself any NFT owned by anyone who gave approval to Golom. The totalAmt can just be set to 0 and we get all NFTs for free.

Tools Used

manual

Recommended Mitigation Steps

Fix: check that the result of ecrecover is not address(0)

require(signaturesigner != address(0))

1- Set a fixed version for Solidity, not a floating version. Remove ^ in pragma solidity ^0.8.11; It should be pragma solidity 0.8.11; GovernerBravo.sol, GolomToken.sol, …

2- No need for ABIEncoderV2 with Solidity 0.8 Remove pragma experimental ABIEncoderV2;

3- File names do not match contract names + there are typos in the filenames GovernerBravo.sol but inside the contract is called GovernorAlpha Either fix the filename or the contract name. If you change the filename, GovernerBravo.sol should be GovernorBravo.sol (with an "o").

Same for Timlock.sol, but the name of the contract is Timelock. Or VoteEscrowDelegation.sol -> VoteEscrow contract.

4- Add 0-address checks in constructors and setter functions. For example: RewardDistributor.sol in constructor https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L74 Or in the constructor of GolomToken: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol#L28

5- Remove console.log in RewardDistributor.sol