Golom contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

When the admin tries to set ve, ve will always be zero address thus the whole reward system won't work properly.

Proof of Concept

When we deploy the RewardDistributor contract, both ve and pendingVoteEscrow are zero addresses.

And after deployment, when the admin calls addVoteEscrow() to update ve, this if condition will always be met, so both ve and pendingVoteEscrow will be zero addresses again.

As a result, the admin can't update ve address thus the system will be useless.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should modify this line like below.

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

Vulnerability details

Impact

There is a case to call the _writeCheckpoint() function with nCheckpoints = 0.

Thus it will revert with uint underflow error here and the delegation logic won't work as expected.

Proof of Concept

Users delegates tokens using delegate() function and this scenario is possible.

1. Alice calls the delegate() function with the new toTokenId param.
2. As toTokenId hasn't been used before, nCheckpoints will be 0.
3. So when the _writeCheckpoint() is called here with nCheckpoints = 0 and it will revert here because of uint underflow error.

At the first time, numCheckpoints will be empty and it can't be added at all because it reverts every time for new delegation.

It means the whole delegation system might be broken.

Also, there is one more issue with this part and I report here together.

Currently, oldCheckpoint is declared as a memory but it should be a storage variable to apply the changes.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should modify this part like below.

if (nCheckpoints > 0 && checkpoints[toTokenId][nCheckpoints - 1].fromBlock == block.number) {
    checkpoints[toTokenId][nCheckpoints - 1].delegatedTokenIds = _delegatedTokenIds;
} else {
    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/core/GolomTrader.sol#L154

Vulnerability details

Impact

The transfer() function is deprecated and it might fail in certain cases.

Proof of Concept

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

1. The claimer smart contract does not implement a payable function.
2. The claimer smart contract does implement a payable fallback that uses more than 2300 gas unit.
3. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend using call() instead of transfer().

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

Use safetransferfrom() instead of transferfrom() for ERC721. According to OpenZeppelin's documentation, transferFrom() is discouraged and use safeTransferFrom() instead.

Lines of code

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

Vulnerability details

Impact

VoteEscrowCore._burn() will revert when an approved user(not owner) calls it.

As a result, merge() and withdraw() functions won't work when msg.sender is not a owner of the token.

Proof of Concept

The _burn() function calls _removeTokenFrom() with msg.sender param but this function works only when _from = owner of the token here.

As a result, the _burn() function will revert when msg.sender isn't the owner of the token to burn.

When I submitted the same issue for the past Velodrome contest in May, there were a few finders but I think it would be lots of finders this time as some wardens can check past issues even though the report hasn't been published yet.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should change this line like below.

_removeTokenFrom(owner, _tokenId);

Low Risk Issues

1. Use safetransferfrom() instead of transferfrom() for ERC721.

According to OpenZeppelin's documentation, transferFrom() is discouraged and use safeTransferFrom() instead.

2. Wrong comments

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

This functions works with orderType 1 and 2.

3. Use safeTransfer(), safeTransferFrom() consistently instead of transfer(), transferFrom() for ERC20 tokens.

• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L151
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L165
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L208-L209

4. Missing events for only functions that change important parameters

• 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/core/GolomTrader.sol#L454
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L58
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L65
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L285
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L291
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L298
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L308
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L260

5. require() should be used instead of assert()

• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L493
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L506
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L519
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L666
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L679
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L861
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L977
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L981
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L991
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1007
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1023
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1110
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1206