Golom contest - arcoun's results

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

1. _writeCheckpoint will fail to insert an initial checkpoint https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L101

Due to solidity 0.8 overflow/underflow protection, accessing checkpoints[toTokenId][nCheckpoints - 1] will throw if nCheckpoints == 0.

As it is not possible to insert the initial checkpoint, _writeCheckpoint will always fail. This issue currently prevents adding a delegation.

Lines of code

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

Vulnerability details

Impact

In the delegate function in VoteEscrowDelegation.sol, there is no verification that a token has been already delegated to an other token.

A malicious user can create several veNFT tokens by splitting and locking his GOL tokens. By delegating each token to the other ones, he can increase his voting power and make a (malicious) proposal to be accepted.

I have rated this finding as Medium because the governance contract is out-of-scope. But voter fraud can be used to gain ownership, update contract settings, transfer available funds, ... This can be High if the impact on the governance contract can be used.

Note: The whole delegation functionnality is currently unusable because the first checkpoint can never be inserted due to an overflow when getting oldCheckpoint in _writeCheckpoint(). A Low-impact finding is related to this bug. This finding assumes that this issue is fixed.

Proof of Concept

• A malicious user has 1000 GOL tokens
• He lock all of them, using 10 create_lock() calls, with 100 GOL tokens each time.
• He now owns 10 veNFT tokens
• He delegate each veNFT token to the 10 veNFT tokens, (100 delegate() calls necessary)
• He now have 10 times more voting power than he should normally have

Update the number of veNFT tokens to create to have as much voting power as necessary to make a proposal be accepted.

Recommended Mitigation Steps

A check must be added to prevent a token to be delegated several times, to different tokens.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L79-L80 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L213-L214

Vulnerability details

Impact

Delegations are tracked using checkpoints stored in the contract's storage.

When delegate() and removeDelegation() are used to add a new checkpoint, the previous checkpoint is also modified because the modification is done on Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1] which is the storage of the previous checkpoint, not a temporary Checkpoint in memory.

By modifying the previous checkpoint, a user can add a delegation after a proposal has been submited and have the related delegation votes taken into account.

I have rated this finding as Medium because it allows voter fraud. The impact can be High on the governance contract, but it is out-of-scope.

Note: The whole delegation functionnality is currently unusable because the first checkpoint can never be inserted due to an overflow when getting oldCheckpoint in _writeCheckpoint(). A Low-impact finding is related to this bug. This finding assumes that this issue is fixed.

Proof of Concept

• Have a veNFT token with at least one delegated token (so there is at least one related checkpoint)
• A voting proposal is submited
• Other veNFT tokens are delegated to the first one, using delegate(), but after the proposal has been submited
• The getPriorVotes() function, called with the proposal timestamp will also return the delegated votes, although they have been delegated after.

Recommended Mitigation Steps

Use a temporary array stored in memory or modify delegatedTokenIds after the new checkpoint has been inserted.

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

7. Usage of deprecated transfer() in payEther() https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L154

The original transfer used to send eth uses a fixed stipend 2300 gas. This was used to prevent reentrancy. However this limit interaction with others contracts that need more than that to process the transaction.

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

In GolomTrader, ether paid to rewards distributor and exchanges will probably be paid to contracts. If one target throw on transfer(), the order will not be processed.

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

2. Use safeTransferFrom() to transfer ERC721 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L236 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L301 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L361 For safety and consistency with ERC1155, ERC721 tokens transfered by GolomTrader should use the safeTransferFrom() function.

Lines of code

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

Vulnerability details

Impact

The safeTransferFrom() function is described in EIP-721 as a function to safely transfer NFTs. As described in EIP-721 and in the NatSpec of the method in VoteEscrowCore.sol, the method must throw if the onERC721Received() function does not return the expected bytes4 signature.

The implementation of safeTransferFrom() in VoteEscrowCore.sol does not verify the returned value.

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

A user may accidentally transfer his NTF to a contract which does not support EIP-721 and permanently lose his asset, although this would have been avoided if the bytes4 signature was verified.

Proof of Concept

• A user wants to transfer his veNFT to an other address
• He calls safeTransferFrom because transferFrom is discouraged (https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L106) as no verification is done on the target.
• The to parameter is set to a contract which do not support EIP-721
• The onERC721Received() call on the destination targets does not throw because the fallback implementation may (for example) handle the call without error.
• His veNTF token is transfered and is now lost

Recommended Mitigation Steps

The bytes4 data, returned by onERC721Received, must be verified against the expected signature.

Note that the expected signature is not bytes4(keccak256("onERC721Received(address,address,uint,bytes)")) as stated in:

• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L589
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L625

The correct signature is bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")) (ie. uint256 instead of uint)

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L216-L267 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L382-L401

Vulnerability details

Impact

The GolomTrader contract does not check if there are remaining ether after an order has been executed. The remaining ether will be permanently lost as it is not automatically returned and there is no function to withdraw ether from the contract.

In fillAsk, remaining ether can be due to:

• user sending more ether than required (https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L217)
• order with o.exchange.paymentAddress == address(0) and o.exchange.paymentAmt != 0 (https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L245)
• order with o.prePayment.paymentAddress == address(0) and o.prePayment.paymentAmt != 0 (https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L248)
• p.paymentAddress == address(0) and p.paymentAmt != 0 (https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L267)

In _settleBalances, remaining ether can be due to:

• order with o.exchange.paymentAddress == address(0) and o.exchange.paymentAmt != 0 (https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L385)
• order with o.prePayment.paymentAddress == address(0) and o.prePayment.paymentAmt != 0 (https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L386)
• p.paymentAddress == address(0) and p.paymentAmt != 0 (https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L401)

Recommended Mitigation Steps

During the handling of an order, the sent ether must be tracked and the remaining ether must be sent back:

• to the calling user in fillAsk
• to the signer in fillBid and fillCriteriaBid

A withdraw() method can also be added for the owner of the contract. This is a good practice, especially when fallback and receive are left available, as it is the case here.

Low Risk Issues

1. _writeCheckpoint will fail to insert an initial checkpoint

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

Due to solidity 0.8 overflow/underflow protection, accessing checkpoints[toTokenId][nCheckpoints - 1] will throw if nCheckpoints == 0.

As it is not possible to insert the initial checkpoint, _writeCheckpoint will always fail. This issue currently prevents adding a delegation.

2. Updating a delegation checkpoint in the same block will not be saved in storage

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

When updating a checkpoint already available for the current block, the modification is done on a memory structure, not on the storage. These modifications are lost after the call is finished.

3. It is not possible to remove a delegation, except for the same token

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

The removeDelegation() function can only be used to remove a delegation of a token on itself. But there is no function to remove a delegation of a token to an other one.

There is a commented removeDelegationByOwner() function in the VoteEscrowDelegation.sol file but it is invalid because it removes the wrong element from delegatedTokenIds (delegatedTokenId instead of ownerTokenId).

4. xxxxRewards() functions should accept a range of epoch

• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L224-L226
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L258
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L273

The 3 functions traderRewards(), exchangeRewards() and stakerRewards() will loop over each epoch (1 day), which will consume more and more ressources each day.

The stakerRewards() will also return an array of epoch elements which will reach 960 bytes after only a month, and 11.4kB after a year.

These functions should accept a range of epoch to limit resource consumption and avoid running out of gas.

5. delegate() and removeDelegation() do not allow token operator

• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L72
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L211

The VoteEscrowCore contract supports setting operator addresses on a token through approve() and setApprovalForAll().

For consistency with other operations on tokens, the delegate() and removeDelegation() should use _isApprovedOrOwner(msg.sender, tokenIf) instead of checking ownerOf(tokenId) == msg.sender

6. validateOrder() should check if order is filled/cancelled before checking for expiration

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L182-L192

If an order has been filled or cancelled, the validateOrder() will return state 1 (deadline reached) once the order expiration timestamp has been reached, although this is not the expected state.

7. Usage of deprecated transfer() in payEther()

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

The original transfer used to send eth uses a fixed stipend 2300 gas. This was used to prevent reentrancy. However this limit interaction with others contracts that need more than that to process the transaction.

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

In GolomTrader, ether paid to rewards distributor and exchanges will probably be paid to contracts. If one target throw on transfer(), the order will not be processed.

Non-critical Issues

1. Use openzeppelin implementation for common functions

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/TokenUriHelper.sol#L5-L63

Base64 library already available (same code) in @openzeppelin/contracts/utils/Base64.sol

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/TokenUriHelper.sol#L128-L149

toString already available (same code) in @openzeppelin/contracts/utils/Strings.sol

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L405-L440

MerkleProof library is available in @openzeppelin/contracts/utils/cryptography/MerkleProof.sol and is already used by the GolomAirdrop contract.

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

ReentrancyGard is available in @openzeppelin/contracts/security/ReentrancyGuard.sol.

2. Use safeTransferFrom() to transfer ERC721

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

For safety and consistency with ERC1155, ERC721 tokens transfered by GolomTrader should use the safeTransferFrom() function.