Golom contest - CRYP70's results

Lines of code

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

Vulnerability details

In the function addVoteEscrow() on line 300 of RewardDistributor.sol there exists an issue where a VoteEscrow contract can never be added or changed. This is because the pendingVoteEscrow address is never set anywhere in the contract including the constructor.

Proof of Concept:

I've included a proof of concept test which will assert that the voteEscrow contract and pendingVoteEscrow will never exist here which may come handy in future unit tests.

Impact

Entitled staker rewards in ETH and GOLOM can never be claimed as there is a check that the voteEscrow on line 175 of RewardsDistributor.sol exists and isn't a null address. I have awarded this a "High" in severity because if the code were to be deployed as is, there would never be a voteEscrow contract.

Recommendations

I recommend that the addVoteEscrow() function is modified to pass the user supplied parameter _voteEscrow if the ve contract doesn't exist as describe by the following:

/// @notice Adds vote escrow contract for multi staker claim
/// @param _voteEscrow Address of the voteEscrow contract
function addVoteEscrow(address _voteEscrow) external onlyOwner {
	if (address(ve) == address(0)) {
		ve = VE(_voteEscrow); // Ensures a voteEscrow contract is added immediately if it's the first
	} else {
		voteEscrowEnableDate = block.timestamp + 1 days;
		pendingVoteEscrow = _voteEscrow;
	}
}

Description and Impact

There exists an issue in the multiStakerClaim() function of the RewardDistributor.sol file on line 172 where any user can claim the staker rewards for another user provided they know the ID of the NFT token. I've awarded this a "Low" in severity because whilst the targeted user's funds are still safely deposited to their wallet address in WETH, their staker NFT is still contained in the voting escrow and I believe the value of their rewards doesn't appreciate overtime if they decide claim early, other users should not have the ability to control staker funds and withdraw them prematurely. This would be rated as a "High" if the attacker were able to limit the potential amount of profits gained by the victim user over time.

This was discovered in the following location: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L172-L210

Recommendations

I recommend adding a simple check after determining who the targeted token belongs to which asserts that the msg.sender is in fact also the token owner.

require(msg.sender == tokenowner, "Unauthorized withdrawal of funds")

Description

During the course of my testing I've noticed that there could be a few extra changes made to the project that may help with the saving of gas costs. See the results of my research below:

++i saves more gas than i++

++i generally costs less gas than i++ or i = i + 1 (about 5 units per increment) because i++ must increment a value and then "return" the old value which means the program may need to hold two numbers in memory. When ++i is used, it will only ever use one number in memory.

See the example below for an simplified illustration:

pragma solidity ^0.8.13;

contract MyFavouriteCounter {

    uint public count;

    function incrementPrefixCount() public returns (uint) {
        count = 1;
        return (++count); // returns 2
    }

    function incrementPostfixCount() public returns (uint) {
        count = 1;
        return (count++); // returns 1
    }
}

Note that this optimization done very well in the VoteEscrowCore.sol contract.

I managed to identify this in the following locations: RewardsDistributor.sol: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L143 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L157 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L180 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L183 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#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 GolomTrader.sol: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L415 VoteEscrowDelegation.sol: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L171 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L189 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L199

<array>.length can be cached as opposed to looking up the length with each iteration

<array>.length can be cached in a variable so the program doesn't need to spend additional gas to determine the length of the target array. I believe memory arrays use 3 gas units through the mload opcode and calldata arrays will use 3 gas units via the calldataload opcode. Observe the usage of gas in the very simple example below with resulting gas units spent (note that this is excluding gas optimisation of the index incrementer and includes gas price spent after reading from storage for both function calls).

pragma solidity 0.8.13;

contract MyFavouriteLooper {

    string[] testarr = ["a", "b"];

    function stored() public { // gas units spent: 23713
        uint256 len = testarr.length;
        for(uint256 i = 0; i < len; i++){

        }
    }

    function notstored() public { // gas units spent: 27479
        for(uint256 i = 0; i < testarr.length; i++){

        }

    }


}

I managed to identify this in the following solidity files: RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L143 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L157 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L180 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L183 GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L415 VoteEscrowDelegation.sol https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L171 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L189 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L199

Reading from storage (sload) with each iteration of an array should be cached whenever possible

Each storage variable read (opcode sload) can become quite pricey when read time and time again during the iteration of an array. When a storage variable is read for the first time (cold access), this can cost 2,100 gas units to execute the transaction. Each time after that (warm access) will cost an additional 100 gas units thereafter. I recommend caching the desired storage slot into a variable and reading the variable directly from there to avoid unnecessary gas costs (3 gas units for each read).

This was observed in the following contracts: RewardDistributor.sol: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L144 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L158 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L273