Golom contest - obront's results

Lines of code

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

Vulnerability details

Impact

When _settleBalances() is called from either the fillBid() or fillCriteriaBid() in the GolomTrader.sol contract, we start by calculating the protocolfee.

uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;

This calculation represents the total protocol fee that should be taken on the full transaction.

When we later pay out msg.sender, we subtract the protocol fee, exchange payment, prepayment, and referrer from the total amount, and then multiply this result by the amount.

payEther((o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) * amount - p.paymentAmt, msg.sender);

Because all of the other variables are calculated on a per-sale basis, they are multiplied by amount to get the final value to send. However, this multiplies the protocol fee by amount twice, which results in msg.sender being paid less.

The remaining amount that isn’t paid to msg.sender isn’t sent elsewhere, and will remain in the contract without a way to withdraw it.

Proof of Concept

Let’s go through an example with the following values:

• totalAmt: 1000
• exchange: 50
• prepayment: 25
• referrer: 25
• amount: 10
• p: 0

In this example, msg.sender is selling 10 ERC1155s for 1000 each, and 100 of that should be going to the other parties.

uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;
= ((1000 * 50) / 10000) * 10
= 50

An additional 50 of the total 10,000 of sales should be going to the protocol, leaving msg.sender with a proper payout of (1000 - 100) * 10 - 50 = 8950.

Instead, the protocol fee is multiplied by the amount again, so the payout specified by the contract is:

payEther((o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) * amount - p.paymentAmt, msg.sender);
=(1000-50-50-25-25) * 10 - 0
=8500

The shortage of 450 is left in the contract, and will effectively be burned.

Tools Used

VSCode, Hardhat

Recommended Mitigation Steps

When calculating the protocolfee on line 381, do not include the amount.

uint256 protocolfee = ((o.totalAmt * 50) / 10000);

Then, when sending the fee to the protocol, multiply the amount in to the payEther argument:

payEther(protocolfee * amount, address(distributor));

Lines of code

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

Vulnerability details

Impact

In the delegate() function in the VoteEscrowDelegation.sol contract, users submit their own tokenId, as well as a tokenId to delegate to, which adds voting power to the delegate.

Delegate power is tracked via the delegatedTokenIds array in the checkpoints mapping, which holds the tokenIds that have been delegated to a given token, and are used to calculate voting power in the getVotes() function.

However, the delegate() function contains no validation that a user’s token isn’t already delegated, which allows any user to bestow unlimited voting power on any other token, including one that they own.

Proof of Concept

Assume User A and User B both own NFTs that have been locked, and therefore have voting power.

User A calls delegate(userAToken, userBToken) to delegate their token to User B’s token, repeating this action n times. On each subsequent call:

• the function confirms that User A is the token owner
• the function confirms that the NFT has more than MIN_VOTING_POWER_REQUIRED
• delegates[tokenId] remains set to User B’s token
• User A’s token is pushed onto the end of User B’s delegatedTokenIds array for the current checkpoint
• _writeCheckpoint() is called, which sets the checkpoint to the new expanded array

Each subsequent call adds an additional instance of User A’s token to the array, so that User B ends up with n instances of User A’s token in the array.

When getVotes(userBToken) is called in the future, the function:

• calls out to _getCurrentDelegated(userBToken), which returns the latest delegatedTokenIds array for User B’s token
• loops through each tokenId on the list, and gets the balanceOfNFT value for that tokenId
• returns the sum of each of the token balances

The result is that User B will have the voting power of balanceOfNFT(userAToken) * n, which can be made arbitrarily high by increasing n.

Tools Used

VSCode, Hardhat

Recommended Mitigation Steps

Add a check to the delegate() function that confirms that the token isn’t already delegated:

require(delegates[tokenId] == address(0))

Alternatively, if you’d like the default behavior of calling delegate() while already delegated to be to overwrite the existing delegation, you could instead use:

if (delegates[tokenId] != address(0)) {
    removeDelegation(tokenId);
}

NOTE: This would require changing removeDelegation() from external to public.

Lines of code

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

Vulnerability details

Impact

In the addFee() function of the RewardDistributor.sol contract, the function begins by checking the total supply of the reward token. If the supply exceeds the cap (1 billion), the function returns before recording any information.

if (rewardToken.totalSupply() > 1000000000 * 10**18) {
    return;
}

This effectively stops the contract from minting additional reward tokens and enforces the supply cap, but it also has the consequence of stopping the protocol from capturing epochTotalFee data or starting new epochs, effectively freezing them in their current state.

These epochs are used by the multiStakerClaim() function to ensure that Eth rewards aren’t claimed twice, with the following protections:

require(claimed[tokenids[tindex]][epochs[index]] == 0, 'cant claim if already claimed');
claimed[tokenids[tindex]][epochs[index]] = 1;

As a result, when the epoch is frozen, Eth rewards will become unclaimable to stakers.

Proof of Concept

Once the supply cap hits 1 billion, all calls to addFee() will result in an immediate return. Therefore the epoch will remain frozen at its current value, as will epochTotalFees[epoch].

Each staker will be able to call multiStakerClaim() one time with that epoch, and will be paid based on the epochTotalFees at the time that addFee was frozen.

After that, there will be no new epochs from which stakers will be able to earn fees.

Tools Used

VSCode, Hardhat

Recommended Mitigation Steps

Move the check in the addFee() function so that it only impacts the logic around the reward token.

After the check is moved, both the epoch and epochTotalFee variables should continue to be set, and the NewEpoch even should continue to be emitted. Collected fees should also be deposited into WETH to allow for distributions.

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

[L-04] payEther() should use .call instead of .transfer https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L154

Use of .transfer is no longer encouraged, as it may fail if the receiver has any logic in their receive() function, due to the 2300 gas consumption limit.

Lines of code

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

Vulnerability details

Impact

In the fillAsk() function of the GolomTrader.sol contract, msg.value is permitted to be higher than the amount to be distributed.

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

When payouts are distributed, the excess funds are not included in the calculations for what should be sent to the signer (or other parties), and are not returned to the sender.

There is no other function in the contract to retrieve these funds, so they’ll remain stuck indefinitely.

Proof of Concept

Let’s designate a variable T which equals o.totalAmt * amount + p.paymentAmt, and represents the total amount of funds that will be distributed upon execution of the function.

The require statement on line 217 requires that msg.value > T.

When ether is paid out, we send:

• distributorfee * amount
• o.exchange.paymentAmt * amount
• o.prePayment.paymentAmt * amount
• o.referrerAmt * amount
• the o.signer payment, which is calculated as (o.totalAmt - distributorfee - exchange payment - prepayment - referrer payment) * amount

At this point, the amount sent equals (o.totalAmt + distributorfee - distributorfee + exchange payment - exchange payment + prepayment - prepayment + referrer - referrer) * amount = o.totalAmt * amount

Finally, the payment amount is sent, bringing the total sent to o.totalAmt * amount + p.paymentAmt, which equals T.

As a result, we are left with msg.value - T in the contract, and no way to reclaim these funds.

Tools Used

VSCode, Hardhat

Recommended Mitigation Steps

The simplest option is to rewrite the require statement to ensure that msg.value is exact:

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

Alternatively, you can add a payEther() call to refund the additional payment to msg.sender once all other funds are distributed:

payEther(msg.value - (o.totalAmt * amount) - p.paymentAmt, msg.sender);

[L-01] Faulty require statement in fillCriteriaBid

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

In the fillCriteriaBid() function of the GolomTrader.sol contract, the require check confirms that the total amount is greater than or equal to the sum of exchange payment, the prepayment, and the referrer payment.

require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt)

This check does not include the extra payment p that should be sent on a successful execution of the order.

When _settleBalances() is called later in the function, it sends funds in the following way:

• Protocol fee is sent to the distributor
• Exchange payment is sent to the exchange
• Prepayment is sent to the prepayment address
• Referrer payment is sent to the referrer
• msg.sender is sent the total amount, minus all other payments (including p)
• The additional payment p is sent to the payment address

In the case that p exists and is greater than the payout that was intended for msg.sender, the function will revert.

[L-02] validateOrder() cannot return 0 for invalid signature

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

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

The validateOrder() function contains two equivalent checks to confirm signature signer matches o.signer. As a result, invalid signatures will always revert and never reach the if statement, eliminating the functionality of returning the correct order status.

NOTE: Do not remove the require statement, or else it will create a bug where any user can cancel any other user’s order.

[L-03] fillAsk() settles NFTs from o.signer and fillBid() settles from msg.sender, which bars operators from acting

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

When fillAsk() is called, we assume that o.signer is the owner of the NFT and call transfer from them to the receiver.

if (o.isERC721) {
    require(amount == 1, 'only 1 erc721 at 1 time');
    ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
} else {
    ERC1155(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId, amount, '');
}

Similarly, when fillBid() is called, we assume the same about msg.sender. This bars operators from using your platform. Instead, o.owner should be a separate field that is used for the transfer, while o.signer is simply checked for permission.

[L-04] payEther() should use .call instead of .transfer

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

Use of .transfer is no longer encouraged, as it may fail if the receiver has any logic in their receive() function, due to the 2300 gas consumption limit.

[L-05] User can trigger unlimited OrderCancelled events

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

Since the cancelOrder() function doesn’t validate based on the status returned by validateOrder(), users will be able to cancel their orders an arbitrary number of times.

This shouldn’t cause any major problems, but will lead to the OrderCancelled() event being emitted each time, which could cause confusion for anyone monitoring the platform.

Instead, put the event emission behind a status check.

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

[L-06] Any funds sent to the GolomTrader or RewardDistributor contracts will be stuck

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

Both GolomTrader.sol and RewardDistributor.sol implement a payable receive function, so they are able to be sent Eth.

receive() external payable {}

However, both contracts lack a way to retrieve those funds.

My recommendation is to eliminate the receive() function. For RewardDistributor.sol, this would require making addFee() payable, and refactoring GolomTrader.sol to send Eth via that function call, which is better aligned with the logical flow of what’s happening.

Alternatively, you could add a rescueEth() function to one or both of the contracts, or use address(this).balance in the epoch fee calculation so that extra funds are automatically distributed along with the rest of the contract fees.

[N-01] Many require statements are missing error messages

Include error messages on require statements. Here is the list of missing require statements from GolomTrader.sol (similar for other contracts):

• GolomTrader.sol:220: require(msg.sender == o.reservedAddress);
• GolomTrader.sol:285: require(o.totalAmt * amount > (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt);
• GolomTrader.sol:291: require(msg.sender == o.reservedAddress);
• GolomTrader.sol:293: require(o.orderType == 1);
• GolomTrader.sol:295: require(status == 3);
• GolomTrader.sol:296: require(amountRemaining >= amount);
• GolomTrader.sol:313: require(o.signer == msg.sender)
• GolomTrader.sol:342: require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);
• GolomTrader.sol:347: require(o.orderType == 2);
• GolomTrader.sol:349: require(status == 3);
• GolomTrader.sol:350: require(amountRemaining >= amount);

[N-02] Remove all logging statements and comments from the contract

The contracts currently import hardhat/console.sol. Remove this import, as well as the commented out console.log statement (line 99 of RewardDistributor.sol) before deploying.

[N-03] secsInDay variable can be replaced with built in “1 days”

The RewardDistributor.sol contract uses a manually added constant secsInDay to calculate 24 * 60 * 60.

Solidity has this same number as a built in option if you simply call 1 days, which makes the code more readable.

[N-04] epochBeginTime should be renamed epochBeginBlock

Since epochBeginTime is tracking the block number, it should be renamed to accurately represent its purpose.

[N-05] Accidental repeated comment for the claimed state variable

In the comments explaining the mappings in RewardDistributor.sol, the explanation for the claimed mapping is a repeat of the previous line, which does not correctly describe the purpose of claimed.

[N-06] Unnecessary return at the end of addFee()

The addFee() function ends with a call to return, which is unnecessary. All functions will automatically return at the end of their body.