Golom contest - zzzitron's results

Lines of code

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

Vulnerability details

Impact

MED - Function could be impacted

The delegate function does not work properly, as the result, vote functionality is impacted

Proof of Concept

• Proof Of concept: the delegate reverts

1. The delegate reverts (The above proof of concept) In the VoteEscrowDelegation, the delegate function calls _writeCheckpoint even when numCheckpoints is zero. When numCheckpoints is zero, _writeCheckpoint tries to access a array element of minus one (line 101), which results to revert with underflow panic. Since the first attempt to delegate fails, nobody can delegate any votes to anybody.

The getVotes will only count the delegated votes (not the balance of its own), nobody has any vote.

2. (Possibly) one can delegate the smae tokenId multiple times I was testing the delegate function for the possibility of multiple delegation, but since the delegation function itself does not work, multiple delegation was not possible to be tested. The vote was counted based on the checkpoints and it does not check the delegates. So, one can delegate multiple times and the same tokenId can be pushed to the delegatedTokenIds and it will be counted as a valid vote for multiple times.

// VoteEscrowDelegation: delegate and _writeCheckpoint

 71     function delegate(uint256 tokenId, uint256 toTokenId) external {
 72         require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
 73         require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
 74
 75         delegates[tokenId] = toTokenId;
 76         uint256 nCheckpoints = numCheckpoints[toTokenId];
 77
 78         if (nCheckpoints > 0) {
 79             Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
 80             checkpoint.delegatedTokenIds.push(tokenId);
 81             _writeCheckpoint(toTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
 82         } else {
 83             uint256[] memory array = new uint256[](1);
 84             array[0] = tokenId;
 85             _writeCheckpoint(toTokenId, nCheckpoints, array);
 86         }
 87
 88         emit DelegateChanged(tokenId, toTokenId, msg.sender);
 89     }
 90
 91     /**
 92      * @notice Writes the checkpoint to store current NFTs in the specific block
 93      */
 94     function _writeCheckpoint(
 95         uint256 toTokenId,
 96         uint256 nCheckpoints,
 97         uint256[] memory _delegatedTokenIds
 98     ) internal {
 99         require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
100
101         Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
102
103         if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
104             oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
105         } else {
106             checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
107             numCheckpoints[toTokenId] = nCheckpoints + 1;
108         }
109     }

Tools Used

hardhat

Recommended Mitigation Steps

The line 101 should be triggered only when the nCheckpoints > 0.

 94     function _writeCheckpoint(
 95         uint256 toTokenId,
 96         uint256 nCheckpoints,
 97         uint256[] memory _delegatedTokenIds
 98     ) internal {
 99         require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
100
103         if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
                Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
                if (oldCheckpoint.fromBlock == block.number) {
104                 oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
                } else {
                    checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
                    numCheckpoints[toTokenId] = nCheckpoints + 1;
                }
105         } else {
106             checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
107             numCheckpoints[toTokenId] = nCheckpoints + 1;
108         }
109     }

<!-- zzzitron 02M -->

Lines of code

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

Vulnerability details

Impact

HIGH - assets can be lost directly.

When fillCriterialBid and fillBid are called for multiple NFTs, the taker of the bid will get less ether then they should get, and the ether will be lost in the GolomTrader.

Proof of Concept

The _settleBalances is counting the protocolfee multiple times. In the line 381, the protocolfee is counted for the amount of NFTs. However upon subtracting the fees from the totalAmt, the protocolfee is again multiplied (line 390, 397). As the result the taker of the deal will get paid less ether then they should be. The ether the taker should get will be lost in the GolomTrader contract as there is no method to rescue the fund.

For example, the totalAmt is 10 ether and amount is 10, the protocolfee is 0.5 ether. For simplicity let's say other fees are zero. The taker of the bid, then, should get 99.5 (= 100 - 0.5). However, the taker will only get 95 ((10 - 0.5) * 10). The difference will be lost in the GolomTrader contract.

// https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L375-L403
// GolomTrader::_settleBalances
// used in fillCriteriaBid and fillBid
// the protocolfee already counts for amount
// but again multiplied in the payEther (line 390, 397)

381         uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;
382         WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);
383         WETH.withdraw(o.totalAmt * amount);
384         payEther(protocolfee, address(distributor));
385         payEther(o.exchange.paymentAmt * amount, o.exchange.paymentAddress);
386         payEther(o.prePayment.paymentAmt * amount, o.prePayment.paymentAddress);
387         if (o.refererrAmt > 0 && referrer != address(0)) {
388             payEther(o.refererrAmt * amount, referrer);
389             payEther(
390                 (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
391                     amount -
392                     p.paymentAmt,
393                 msg.sender
394             );
395         } else {
396             payEther(
397                 (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
398                 msg.sender
399             );
400         }

Tools Used

None

Recommended Mitigation Steps

Subtract protocolfee only once.

// GolomTrader::_settleBalances
// mitigation suggestion

387         if (o.refererrAmt > 0 && referrer != address(0)) {
388             payEther(o.refererrAmt * amount, referrer);
389             payEther(
390                 (o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
391                     amount - protocolfee -
392                     p.paymentAmt,
393                 msg.sender
394             );
395         } else {
396             payEther(
397                 (o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - protocolfee - p.paymentAmt,
398                 msg.sender
399             );
400         }

<!-- zzzitron 00H -->

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

L01: Usage of transfer to send eth It is recommended to use call instead of transfer due to fixed gas stipend. In the GolomTrader, transfer is used to pay ether.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L58-L72 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L444-L457 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L298-L311

Vulnerability details

Impact

MED - Function could be impacted

As the timelock does not work as supposed to work, the owner of the contract can bypass timelock.

• effected Functions:
  • GolomToken: setMinter, executeSetMinter
  • GolomTrader: setDistributor, executeSetDistributor
  • RewardDistributor: addVoteEscrow, executeAddVoteEscrow

Proof of Concept

• GolomTrader::it can bypass timelock poc
• GolomToken::setMinter it should set the minter with timelock poc

The first poc shows to bypass timelock for GolomTrader::setDistributor. The same logic applies for the RewardDistributor::addVoteEscrow. 0. The setDistributor was called once in the beforeEach block to set the initial distributor. For this exploit to work, the setDistributor should be called only once. If setDistributor was called more than once, one can set the distributor to zero address (with timelock like in the GolomToken case, then set to a new distributor after that)

1. reset distributor to zero address without timelock by calling executeSetDistributor
2. set a new distributor without timelock by calling setDistributor
3. Rinse and repeat: as long as setDistributor is not called multiple times in row, the owner can keep setting distributor without timelock.

A little bit different variation of timelock bypass was found in the GolomToken. Although the owner should wait for the timelock to set the minter to zero address, but after that, the owner can set to the new minter without waiting for a timelock. Since the meaning of timelock is to let people know the new minter's implementation, if the owner can bypass that, the timelock is almost meaningless. The exploitation steps: the second proof of concept

1. call setMineter with zero address
2. wait for the timelock
3. call executeSetMineter to set the minter to zero address
4. now the onwer can call setMineter with any address and call executeSetMinter without waiting for the timelock

The owner can call executeSetdistributor even though there is no pendingDistributor set before. Also, setDistributor sets the new distributor without timelock when the existing distributor's address is zero.

// GolomTrader
// almost identical logic was used in `RewardDistributor` to addVoteEscrow
// similar logic was used in `GolomToken` to `setMineter`


444     function setDistributor(address _distributor) external onlyOwner {
445         if (address(distributor) == address(0)) {
446             distributor = Distributor(_distributor);
447         } else {
448             pendingDistributor = _distributor;
449             distributorEnableDate = block.timestamp + 1 days;
450         }
451     }
452
453     /// @notice Executes the set distributor function after the timelock
454     function executeSetDistributor() external onlyOwner {
455         require(distributorEnableDate <= block.timestamp, 'not allowed');
456         distributor = Distributor(pendingDistributor);
457     }

Tools Used

None

Recommended Mitigation Steps

To mitigate, execute functions can check whether pendingDistributor is not zero. It will ensure that the setters are called before executing them, as well as prevent to set to zero addresses.

<!-- zzzitron 01M -->

Golom QA Reports

Summary

L00: Missing zero address check

• In GolomTrader, if the distributor is zero, the fee will be sent to zero address:

  • distributor is not initialized in the constructor, also it can be set to zero with setDistributor
  • 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#L384
  • Also fillAsk, fillBid and fillCriteriaBid allows to send ether to zero address if the maker and taker sets so.

• check for _governance (the owner)

  • GolomToken::constructor : https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L29
  • RewardDistributor::constructor : https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L83
  • GolomTrader::constructor : https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L94

L01: Usage of transfer to send eth

It is recommended to use call instead of transfer due to fixed gas stipend. In the GolomTrader, transfer is used to pay ether.

L02: Unreachable code

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

Since it reverts when signaturesigner is not o.signer, the part of code in the line 178-180 will never be reached.

// GolomTrader::validateOrder

        require(signaturesigner == o.signer, 'invalid signature');
        if (signaturesigner != o.signer) {
            return (0, hashStruct, 0);
        }

L03: RewardToken can mint over 1 billion

Since the check for the totalSupply is happening before minting, it is possible to mint slightly more than 1 billion.

L04: Not following standard

_balance and thus balanceOf do not throw for zero address, as standard ERC721

// from https://eips.ethereum.org/EIPS/eip-721
// interface ERC721

    /// @notice Count all NFTs assigned to an owner
    /// @dev NFTs assigned to the zero address are considered invalid, and this
    ///  function throws for queries about the zero address.
    /// @param _owner An address for whom to query the balance
    /// @return The number of NFTs owned by `_owner`, possibly zero
    function balanceOf(address _owner) external view returns (uint256);

L05: Over paid eth will be lost

In GolomTrader, over paid eth is not returned to the taker and it will be lost in the contract.

L06: Not sufficient check

• fillBid
• fillCriteriaBid

The checks on totalAmt is not sufficient for fillBid and fillCriteriaBid:

• fillBid: protocolfee is not accounted
• fillCriteriaBid: protocolfee and payment to taker's Payment is not accounted However, if the totalAmt is not sufficient, the call will revert in the _settleBalances with underflow

// fillBid check
        require(
            o.totalAmt * amount >
                (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt
        );

// fillCriteriaBid check
        require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);

NC00: Usage of magic number

• GolomToken: the constant for the 150M : https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L44
• GolomToken: the constant for 62.5M : https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L52
• RewardDistributor: startTime : https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L84
• GolomTrader: the logic to calculate protocol fee is repeated with hardcoded ratio. It is easier to understand to use a helper function or a constant value to calculate them:
  • 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#L381

NC01: API in the hardhat config

In the hardhat.config.ts file, an API key for alchemy is hardcoded and it seems not to be disabled. Malicious actors can search through git commit history, so the API key should be retired.

<!-- zzzitron QA -->