Golom contest - GiveMeTestEther's results

Lines of code

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

Vulnerability details

Impact

An attacker can multiply his voting power for a single tokenId by 499 times by self-delegating. (can at least multiple by 499 times (https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L99) )

Proof of Concept

1. user locks some $GOLOM tokens by calling VoteEscrow.create_lock_for() and gets a tokenId
   1. https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L959
2. user calls VoteEscrowDelegation.delegate(tokenId, tokenId) 3. https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L71
3. Because there is now initial delegation for this token the else branch is triggered
   1. https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L85
4. user call again user calls VoteEscrowDelegation.delegate(tokenId, tokenId) for same tokenId. This time we have a checkpoint for this tokenId and the if statement is true
   1. https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L79-L81
5. L80 pushes again the same tokenId and this checkpoint is written on L85
6. an attacker could call this multiple times to have the same tokenId pushed multiple time to checkpoint.delegatedTokenIds array
7. the getVotes() call would call uint256[] memory delegated = _getCurrentDelegated(tokenId); and get the checkpoints[tokenId][nCheckpoints - 1].delegatedTokenIds that hold our same tokenId multiple times
   1. the for-loop iterates over the delegatedTokensId votes = votes + this.balanceOfNFT(delegated[index]); whereas delegated[index] is always the same tokenId => this is the same as votes = this.balanceOfNFT(tokenId) * delegated.length (whereas delegated.length is max 499) => attacker can multiple his own voting power by 499 times votes = this.balanceOfNFT(tokenId) * 499
   2. https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L168

Tools Used

VSC

Recommended Mitigation Steps

• check if tokenId is already delegated and remove the delegation beforehand
• it seems that some structures were created to handle this but not fully implemented see: https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L75

Lines of code

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

Vulnerability details

Impact

After reaching a totalSupply of over 1 Billion $GOLOM tokens, the platform fees (0.5% of the trade amount in ETH) can not be redeemed by the stakers anymore and are locked in the RewardDistributor contract.

Proof of Concept

1. assumption the totalSupply of $GOLOM tokens is over 1 Billion
2. fillAsk() / fillBid() / fillCriteriaBid() from the GolomTrader contract will trigger the distributor.addFee() that sends the 0.5% protocol fees to the RewardDistributor contract
   • https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L402
   • https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L269
3. the if statement of the addFee() in the RewardDistributor function if (rewardToken.totalSupply() > 1000000000 * 10**18) { is true and therefore returns directly (no increase in epoch and no fee is set/added to epochTotalFee[epoch]
   • https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L100
4. There is no function to claim the ETH rewards/balance (still in ETH not in WETH)

Tools Used

VSC

Recommended Mitigation Steps

• add a function to claim the ETH balance of the RewardDistributor contract with a onlyOwner modifier. There is no rug-pull vector because addFee() function directly swaps the ETH to WETH. This also allows to rescue ETH funds if they are accidentally sent to this contract. OR
• deactivate the addFee() call from the GolomTrader smart contract if the $GOLOM has reached full supply.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L65-L72 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L444 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L298

Vulnerability details

Impact

Time lock special case: After the <b>time lock address</b> is set back to the <b>0x0</b> address the <b>1 days</b> time lock period doesn't take effect and can be set by the onlyOwner anytime to a different address. There are also no events triggered by time lock functions such that user could listen & react to these events.

Affected contracts.

• GolomToken.sol
• GolomTrader.sol
• RewardDistributor.sol

Proof of Concept

Example GolomToken.sol:

1. If <b>minter</b> was set back to the <b>0x0 address</b> and 1 days time lock period passed
2. <b>onlyOwner</b> executes <b>setMinter(newMinterAddress)</b>
3. <b>onlyOnwer</b> executes <b>executeSetMinter()</b> immediately after
   • because <b>minter == address(0)</b> is true, it <b>minter</b> gets set immediately to <b>setMinter(newMinterAddress)</b>

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L58-L61

/// @notice sets the minter with timelock, once setup admin needs to call executeSetMinter()
/// @param _minter Address of the new minter

function setMinter(address _minter) external onlyOwner {
	pendingMinter = _minter;
	minterEnableDate = block.timestamp + 1 days;
}

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L65-L72

/// @notice Executes the set minter function after the timelock
/// @dev If being called first time, there won't be any timelock
function executeSetMinter() external onlyOwner {
	if (minter == address(0)) {
		minter = pendingMinter;
	} else {
		require(minterEnableDate <= block.timestamp, 'GolomToken: wait for timelock');
		minter = pendingMinter;
	}
}

Tools Used

VSC

Recommended Mitigation Steps

1. use events for critical protocol functions
2. use a different logic for setting the initial address or
3. insert a check 0x0 address in the setter function

Lines of code

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

Vulnerability details

Impact

msg.value send to GolomoTrader.sol by calling fillAsk can be greater than the necessary amount and get therefore locked in the GolomTrader because it is not accounted for in the payEther() calls

Proof of Concept

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

Tools Used

VSC

Recommended Mitigation Steps

• instead of >= use == comparison

Lines of code

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

Vulnerability details

Impact

RewardDistributor.stakerRewards() fucntion call will always result in a division by zero error if there is an epoch such that ve.totalSupplyAt(epochBeginTime[index]) is zero (no staked amount).

This also affects RewardDistributor.multiStakerClaim() in a limited fashion, because a caller can set the epochs as a functions parameter. But this can therefore also break the assumptions of other protocol integrations that are using this function. (e.g. claim all epoch rewards between the highest epoch plus 1 of the last call to the current (epoch-1)).

Proof of Concept

RewardDistributor.stakerRewards(): Iterates from epoch 0 to (epoch-1) and calls ve.totalSupplyAt(epochBeginTime[index]). If there is at least one epoch w/o any staked amount this will revert with a division by 0

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

Tools Used

VSC

Recommended Mitigation Steps

• add a zero check