Golom contest - 0x1f8b's results

Lines of code

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

Vulnerability details

Impact

The addVoteEscrow method is poorly implemented and denies service..

Proof of Concept

The addVoteEscrow method sets the value of ve to pendingVoteEscrow as long as the ve is empty, instead of using the received argument (_voteEscrow) since pendingVoteEscrow is empty, it becomes impossible modify ve mediate that method.

    function addVoteEscrow(address _voteEscrow) external onlyOwner {
        if (address(ve) == address(0)) {
           ve = VE(_voteEscrow);
           ve = VE(pendingVoteEscrow);
        }
...

Recommended Mitigation Steps

Fix the method like following:

    function addVoteEscrow(address _voteEscrow) external onlyOwner {
        if (address(ve) == address(0)) {
+           ve = VE(_voteEscrow);
-           ve = VE(pendingVoteEscrow);
        } else {
            voteEscrowEnableDate = block.timestamp + 1 days;
            pendingVoteEscrow = _voteEscrow;
        }
    }

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

5. Ether could be blocked

The user's ether could be locked and unrecoverable.

Because to transfer ether the .transfer method (which is capped at 2300 gas) is used instead of .call which is limited to the gas provided by the user. If a contract that has a fallback method more expensive than 2300 gas, creates an offer, it will be impossible for this contract cancel the offer or receive funds back to that address.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected source code:

• GolomTrader.sol#L154

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

6. Fork chain stored

If there is a hard fork, the EIP712_DOMAIN_TYPEHASH is not computed. After initialization, the variable EIP712_DOMAIN_TYPEHASH in contract GolomTrader is cached in the contract storage and does not change. However, if a hard fork takes place after the contract deployment, the block would cause the domain to become invalid on one of the forked chains. chainid is now different.

Affected source code:

• GolomTrader.sol#L96-L109

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

• Low
  • 1. Outdated compiler
  • 2. Lack of check
  • 3. Lack of ACK during owner change
  • 4. Ensure that ecrecover returns is not address0
  • 5. Ether could be blocked
  • 6. Fork chain stored
  • 7. reentrancy pattern
• Non-Critical
  • 8. Codding style

Low

1. Outdated compiler

The pragma version used is:

pragma solidity 0.8.11;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.15

2. Lack of check

The following lines requires a valid address as an argument, in some of them, once the value is set, it cannot be changed again, so it is mandatory to check if these values are as expected.

Affected source code for address(0):

• GolomToken.sol#L28
• GolomToken.sol#L52
• RewardDistributor.sol#L287
• GolomTrader.sol#L444
• VoteEscrowCore.sol#L868s

Integer range:

• VoteEscrowDelegation.sol#L261

3. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• GolomToken.sol#L14
• VoteEscrowDelegation.sol#L20
• RewardDistributor.sol#L43
• GolomTrader.sol#L40

4. Ensure that ecrecover returns is not address(0)

The method ecrecover returns address(0) when the signature is wrong, so if a user use address(0) as a o.signer the return will be true.

Affected source code:

• GolomTrader.sol#L177

5. Ether could be blocked

The user's ether could be locked and unrecoverable.

Because to transfer ether the .transfer method (which is capped at 2300 gas) is used instead of .call which is limited to the gas provided by the user. If a contract that has a fallback method more expensive than 2300 gas, creates an offer, it will be impossible for this contract cancel the offer or receive funds back to that address.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected source code:

• GolomTrader.sol#L154

6. Fork chain stored

If there is a hard fork, the EIP712_DOMAIN_TYPEHASH is not computed. After initialization, the variable EIP712_DOMAIN_TYPEHASH in contract GolomTrader is cached in the contract storage and does not change. However, if a hard fork takes place after the contract deployment, the block would cause the domain to become invalid on one of the forked chains. chainid is now different.

Affected source code:

• GolomTrader.sol#L96-L109

7. reentrancy pattern

Although it has not been possible to exploit the reentrancy attack, the logic of the methods named below, make the changes of the validation flags after a method that allows reentrancy, it is convenient to modify the flags before the external calls to contracts.

For example, you could call merge or similar functions that don't have nonreentrant, so it's better to apply the following changes:

-       assert(IERC20(token).transfer(msg.sender, value));
        // Burn the NFT
        _burn(_tokenId);
+       require(IERC20(token).transfer(msg.sender, value));

Affected source code:

• VoteEscrowCore.sol#L1026

Non-Critical

8. Codding style

Use 1 days instead of a multiplication:

• RewardDistributor.sol#L57

The following line is not required so it's better to remove the last return:

• RewardDistributor.sol#L137

• Gas
  • 1. Use Custom Errors instead of Revert Strings to save Gas
  • 2. There's no need to set default values for variables
  • 3. Don't use the length of an array for loops condition
  • 4. ++i costs less gas compared to i++ or i += 1
  • 5. Gas saving using immutable
  • 6. Use constants
  • 7. delete optimization
  • 8. Reorder structure layout
  • 9. Improve logic
  • 10. Use require instead of assert
  • 11. Avoid unused returns
  • 12. Avoid creating unnecessary methods

Gas

1. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described above.

Affected source code:

• GolomToken.sol#L24
• GolomToken.sol#L43
• GolomToken.sol#L51
• GolomToken.sol#L69
• VoteEscrowDelegation.sol#L72-L73
• VoteEscrowDelegation.sol#L99
• VoteEscrowDelegation.sol#L130
• VoteEscrowDelegation.sol#L211
• VoteEscrowDelegation.sol#L239
• RewardDistributor.sol#L173
• RewardDistributor.sol#L181-L185
• RewardDistributor.sol#L292
• RewardDistributor.sol#L309
• GolomTrader.sol#L211-L217
• GolomTrader.sol#L222-L235
• GolomTrader.sol#L299
• VoteEscrowCore.sol#L928-L929
• VoteEscrowCore.sol#L945-L946
• VoteEscrowCore.sol#L982-L983
• VoteEscrowCore.sol#L996-L999
• VoteEscrowCore.sol#L1008-L1011
• VoteEscrowCore.sol#L1082

2. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• VoteEscrowDelegation.sol#L147
• VoteEscrowDelegation.sol#L170-L171
• VoteEscrowDelegation.sol#L188-L189
• RewardDistributor.sol#L142-L143
• RewardDistributor.sol#L156-L157
• RewardDistributor.sol#L175-L176
• RewardDistributor.sol#L180
• RewardDistributor.sol#L183
• RewardDistributor.sol#L222-L223
• RewardDistributor.sol#L226
• RewardDistributor.sol#L257-L258
• RewardDistributor.sol#L272-L273
• GolomTrader.sol#L415
• VoteEscrowCore.sol#L697-L698
• VoteEscrowCore.sol#L745
• VoteEscrowCore.sol#L1042-L1044
• VoteEscrowCore.sol#L1113-L1115
• VoteEscrowCore.sol#L1133-L1134
• VoteEscrowCore.sol#L1167

3. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• VoteEscrowDelegation.sol#L171
• VoteEscrowDelegation.sol#L189
• VoteEscrowDelegation.sol#L199
• RewardDistributor.sol#L143
• RewardDistributor.sol#L157
• RewardDistributor.sol#L180
• RewardDistributor.sol#L183
• GolomTrader.sol#L415

4. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• VoteEscrowDelegation.sol#L171
• VoteEscrowDelegation.sol#L189
• VoteEscrowDelegation.sol#L199
• RewardDistributor.sol#L143
• RewardDistributor.sol#L157
• RewardDistributor.sol#L180
• RewardDistributor.sol#L183
• RewardDistributor.sol#L226
• RewardDistributor.sol#L258
• RewardDistributor.sol#L273
• GolomTrader.sol#L415

5. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• RewardDistributor.sol#L46
• RewardDistributor.sol#L68-L69

6. Use constants

Use constant instead of storage/computation values in:

• RewardDistributor.sol#L100
• GolomTrader.sol#L45

Cache keccak256 results:

• GolomTrader.sol#L116
• GolomTrader.sol#L131-L133

7. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

• RewardDistributor.sol#L149
• RewardDistributor.sol#L163
• VoteEscrowCore.sol#L469-L471
• VoteEscrowCore.sol#L483-L485
• VoteEscrowCore.sol#L508
• VoteEscrowCore.sol#L880

8. Reorder structure layout

The following structs could be optimized moving the position of certains values in order to save slot storages:

• GolomTrader.sol#L47-L65

    struct Order {
        address collection; // NFT contract address
        uint256 tokenId; // order for which tokenId of the collection
        address signer; // maker of order address
        uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root
        uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount
        Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt
        Payment prePayment; // another payment , can be used for royalty, facilating trades
-       bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
        uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
        uint256 refererrAmt; // amt to pay to the address that helps in filling your order
        bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
        address reservedAddress; // if not address(0) , only this address can fill the order
+       bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
+       uint8 v;
        uint256 nonce; // nonce of order usefull for cancelling in bulk
        uint256 deadline; // timestamp till order is valid epoch timestamp in secs
-       uint8 v;
        bytes32 r;
        bytes32 s;
    }

9. Improve logic

The following clauses are OR closures, but due to their implementation, all possible paths are validated.

• VoteEscrowCore.sol#L439-L442

    function _isApprovedOrOwner(address _spender, uint256 _tokenId) internal view returns (bool) {
        address owner = idToOwner[_tokenId];
-       bool spenderIsOwner = owner == _spender;
-       bool spenderIsApproved = _spender == idToApprovals[_tokenId];
-       bool spenderIsApprovedForAll = (ownerToOperators[owner])[_spender];
-       return spenderIsOwner || spenderIsApproved || spenderIsApprovedForAll;
+       return (owner == _spender) || (_spender == idToApprovals[_tokenId]) || (ownerToOperators[owner])[_spender];
    }

• VoteEscrowCore.sol#L650-L652

    function approve(address _approved, uint256 _tokenId) public {
        address owner = idToOwner[_tokenId];
        // Throws if `_tokenId` is not a valid NFT
        require(owner != address(0));
        // Throws if `_approved` is the current owner
        require(_approved != owner);
        // Check requirements
-       bool senderIsOwner = (idToOwner[_tokenId] == msg.sender);
-       bool senderIsApprovedForAll = (ownerToOperators[owner])[msg.sender];
-       require(senderIsOwner || senderIsApprovedForAll);
+       require((idToOwner[_tokenId] == msg.sender) || (ownerToOperators[owner])[msg.sender]);
        // Set the approval
        idToApprovals[_tokenId] = _approved;
        emit Approval(owner, _approved, _tokenId);
    }

10. Use require instead of assert

The assert() and require() functions are a part of the error handling aspect in Solidity. Solidity makes use of state-reverting error handling exceptions. This means all changes made to the contract on that call or any sub-calls are undone if an error is thrown. It also flags an error.

They are quite similar as both check for conditions and if they are not met, would throw an error.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

Affected source code:

• VoteEscrowCore.sol#L493
• VoteEscrowCore.sol#L506
• VoteEscrowCore.sol#L519
• VoteEscrowCore.sol#L666
• VoteEscrowCore.sol#L679
• VoteEscrowCore.sol#L861
• VoteEscrowCore.sol#L1023
• VoteEscrowCore.sol#L1110

11. Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• VoteEscrowCore.sol#L677

12. Avoid creating unnecessary methods

Method creation has an extra cost, a method to return the height of the block is not required.

• VoteEscrowCore.sol#L911