Golom contest - CertoraInc's results

Lines of code

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

Vulnerability details

Impact

The _writeCheckpoint function will revert if nCheckpoints == 0 because it will try to calculate nCheckpoints - 1 which will revert.

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

    if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
        oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
    } else {
        checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
        numCheckpoints[toTokenId] = nCheckpoints + 1;
    }
}

This will cause the delegate function to revert on the first delegation, because the user won't have any checkpoints, which will lead to the first delegation to revert and to the delegation mechanism to not function at all.

Proof of Concept

Alice tries to delegate the voting power from tokenId 1 to tokenId 2 by calling VoteEscrowDelegation.delegate(1, 2). It's the first time that tokenId is delegated to tokenId 2 so numCheckpoints[toTokenId] == 0.

Now, the delegate function will call _writeCheckpoint, which will try to access checkpoints[toTokenId][nCheckpoints - 1], but this will make the function revert because nCheckpoints == 0.

Alice won't be able to delegate her tokenId's votes to another tokenId, and the delegate mechanism simply won't work.

Tools Used

Manual audit (VS Code & my mind)

Recommended Mitigation Steps

Access checkpoints[toTokenId][nCheckpoints - 1] only if nCheckpoints > 0. This is a possible fix for that issue:

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

    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/vote-escrow/VoteEscrowDelegation.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L211

Vulnerability details

Impact

The VoteEscrowDelegation._transferFrom function won't work because it calls this.removeDelegation(_tokenId). The removeDelegation function is external, so when the call is done by this.removeDelegation(_tokenId) msg.sender changes to the contract address.

This causes the check in the `` function to (most likely) fail because the contract is not the owner of the NFT, and that will make the function revert. require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

Tools Used

Manual audit (VS Code & my mind)

Recommended Mitigation Steps

Make the removeDelegation function public and call it without changing the context (i.e. without changing msg.sender to the contract's address).

Lines of code

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

Vulnerability details

Impact

The removeDelegation function removes tokenId from it's own _delegatedTokenIds array, but the _delegatedTokenIds contains the NFTs that were delegated to tokenId. So basically this function removes tokenId from the array of the tokenIds that were delegated to him, instead of removing tokenId from the _delegatedTokenIds array of the token id that tokenId was delegated to.

This can cause an attacker delegating his NFT to another NFT he owns, and selling this NFT but still own the votes of it.

Proof of concept

An attacker owns two NFTs - let’s assume he owns token ids 1 and 2.

The attacker delegates the votes of token id 1 to token id 2 by calling delegate(1, 2).

The attacker now sells the NFT with token id 1 to another user (using the GolomTrader contract for example).

The sell triggers the transferFrom function, which eventually calls removeDelegation, but this function will (try to) remove the tokenId from the _delegatedTokenIds array. This array will contain all the token ids that are delegated to token id 1 (we can assume it is empty), but it wont remove token id 1 from the _delegatedTokenIds array of token id 2, and that will make the NFT's votes still being delegated to token id 2.

The attacker can use this attack vector to deceive users to buy his NFT and still use its votes.

Tools Used

Manual audit (VS Code & my mind)

Recommended Mitigation Steps

Instead of (trying to) remove the transferred token id from its own _delegatedTokenIds array, remove it from all the arrays of the token ids that it is delegated to.

A way to implement this mechanism can add an array to every token id that will contain the token ids that this token id is delegated to and update it in the delegate function. That way you can remove the tokenId from all the arrays of the token ids that it is delegated to.

Lines of code

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

Vulnerability details

Impact

Some contracts can contain some logic in their receive/fallback function, and these kind of contracts won't be able to use the GolomTrader contract because of the usage of transfer which limits the gas to 2300.

Proof of concept

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Manual audit (VS Code & my mind)

Recommended Mitigation Steps

Use a low level call instead of the transfer function to transfer ETH to other addresses.

Lines of code

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

Vulnerability details

Impact

The transferFrom function is used to transfer ERC721s, but this is unsafe. It can lead for example to an NFT getting stuck in a contract which doesn't have a way to get it out.

Proof of concept

OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible.

Tools Used

Manual audit (VS Code & my mind)

Recommended Mitigation Steps

Use safeTransferFrom instead of transferFrom to assure that the only contract that can accept the NFT is a contract that has a way to handle the NFT transfer, and this is indicated by implementing the onERC721Received function.

Lines of code

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

Vulnerability details

Impact

A user that sent more ether than he needed to in order to fill an order will lose the excess ether. This can happen if for some reason the total amount in the order is greater than what the user actually needs to pay. In that case the user must assign more ether that will be used to the transaction, but the remaining ether will remain in the contract without a way for the user to get it back.

Tools Used

Manual audit (VS Code & my mind)

Recommended Mitigation Steps

Return the excess ether amount to the user in the end of the fillAsk function

QA Report

• Pragmas should be locked to a specific compiler version, to avoid contracts getting deployed using a different version, which may have a greater risk of undiscovered bugs - in GolomToken it's ^0.8.11 while in the other contracts it is 0.8.11
• Strings are broken into 32 byte chunks for operations. Revert error strings over 32 bytes therefore consume extra gas than shorter strings, as documented publicly. Reducing revert error strings to under 32 bytes decreases deployment time gas and runtime gas when the revert condition is met. Alternatively, use custom errors, introduced in Solidity 0.8.4.

VoteEscrowDelegation

• Comment on line 68 needs to be completed. /// @notice Explain to an end user what this does
• Missing underscore in the name of the removeElement internal function.
• Redundant initialization - variables in solidity are already initialized to their default value, and initializing them to the same value actually costs more gas. This happens in multiple places:
  • line 50 - the initialization of MIN_VOTING_POWER_REQUIRED
  • line 147 - the initialization of lower
  • line 170, 188 - the initialization of votes
  • lines 171, 189 - the initialization of index
• Not emitting event on changes to the contract (for example when changing MIN_VOTING_POWER_REQUIRED or adding/removing delegations).

RewardDistributor

• Some rewards can get lost in the addFees function by rounding errors in the addFee function.

  rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
  rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;

  A fix to that issue is to calculate one of the expressions, and calculate the other one by subtracting it from (tokenToEmit - stakerReward):

  uint remainingRewards = (tokenToEmit - stakerReward); // caching the result to avoid calculating it twice
  uint rewardTrader = (remainingRewards * 67) / 100;
  rewardTrader[epoch] = rewardsTrader; // 2/3 of remainingRewards
  rewardExchange[epoch] = remainingRewards - rewardsTrader; // 1/3 of remainingRewards

• Forgot to remove a comment on line 99 at the addFee function
• Some typos in the comments:
  • line 168 - "there" instead of "their"
  • line 111 - "begiining" and "begining" instead of beginning
• First epoch's VE balance will be counted twice - both on epoch 0 and 1.
• The stakerRewards function will return (in the array return value) 0 for epochs that are not claimed and 1 for epochs that are claimed, when it should do the opposite.
• Redundant return statement at the end of the addFee function (line 137)

GolomTrader

• Missing zero address check in the constructor - the constructor doesn't check that _governance != address(0)
• Missing underscore in the name of the payEther internal function
• Incomplete comment before the validateOrder function (line 160) /// OrderStatus = 1 , if deadline has been This comment should be "OrderStatus = 1 , if deadline has passed"
• Line 177 or lines 178-180 are redundant - they check the same thing twice.

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

  If the wanted behavior is to revert in the case where signaturesigner != o.signer, then lines 178-180 are redundant. If the wanted behavior is to return 0, then line 177 is redundant. In any way, one of this checks is redundant.
• Not emitting event on changes to the contract (for example when changing the distributor).
• Fee can be zero on small values ((o.totalAmt * 50) / 10000) will be zero when o.totalAmt * 50 < 10000), i.e. when o.totalAmt < 200)
• Line 286 should be >= instead of >, because it can be possible that

  o.totalAmt * amount == (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt