Golom contest - Deivitto's results

An NFT marketplace that offers the lowest industry fee, a publicly available order-book along with analytical tools.

General Information

Platform: Code4rena

Start Date: 26/07/2022

Pot Size: $75,000 USDC

Total HM: 29

Participants: 179

Period: 6 days

Judge: LSDan

Total Solo HM: 6

Id: 148

League: ETH

Golom

Findings Distribution

Researcher Performance

Rank: 48/179

Findings: 3

Award: $262.24

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Golom

Findings Information

🌟 Selected for report: cloudjunky

Also found by: 0x1f8b, 0x4non, 0x52, 0xDjango, 0xHarry, 0xNazgul, 0xNineDec, 0xf15ers, 0xsanson, 0xsolstars, 8olidity, Bnke0x0, CertoraInc, Chom, Deivitto, Dravee, GalloDaSballo, GimelSec, IllIllI, Jmaxmanblue, JohnSmith, Jujic, Kenshin, Krow10, Lambda, MEP, Noah3o6, RedOneN, Ruhum, StErMi, StyxRave, TomJ, Treasure-Seeker, TrungOre, _Adam, __141345__, arcoun, asutorufos, bardamu, bearonbike, bin2chen, brgltd, bulej93, c3phas, cRat1st0s, carlitox477, cccz, codexploder, cryptonue, cryptphi, cthulhu_cult, dharma09, dipp, djxploit, durianSausage, ellahi, giovannidisiena, hansfriese, horsefacts, hyh, immeas, indijanc, jayjonah8, jayphbee, joestakey, kenzo, kyteg, ladboy233, minhquanym, navinavu, obront, oyc_109, peritoflores, rbserver, reassor, rokinot, rotcivegaf, saian, scaraven, shenwilly, simon135, sseefried, teddav, zzzitron

Awards

0.0037 USDC - $0.00

Labels

bug

duplicate

2 (Med Risk)

old-submission-method

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1023 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L154 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L151 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L165 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L208 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L209

Vulnerability details

Use of transfer() instead of call() to send eth

Impact

Use of transfer() might render ETH impossible to withdraw because after istanbul hardfork, there is increases in the gas cost of the SLOAD operation and therefore breaks some existing smart contracts.Those contracts will break because their fallback functions used to consume less than 2300 gas, and they’ll now consume more, since 2300 the amount of gas a contract’s fallback function receives if it’s called via Solidity’s transfer() or send() methods.

Any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.

References

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/ https://blog.openzeppelin.com/opyn-gamma-protocol-audit/

Proof of Concept

Recommended Mitigation Steps

Use call() to send eth

#0 - KenzoAgada
2022-08-03T14:02:40Z

Duplicate of #343

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x4non, 0x52, 0xA5DF, 0xDjango, 0xLovesleep, 0xNazgul, 0xNineDec, 0xSmartContract, 0xackermann, 0xc0ffEE, 0xf15ers, 0xmatt, 0xsanson, 0xsolstars, 8olidity, AuditsAreUS, Bahurum, Bnke0x0, CRYP70, CertoraInc, Ch_301, Chom, CryptoMartian, Deivitto, DevABDee, Dravee, ElKu, Franfran, Funen, GalloDaSballo, GimelSec, GiveMeTestEther, Green, JC, Jmaxmanblue, JohnSmith, Jujic, Junnon, Kenshin, Krow10, Kumpa, Lambda, MEP, Maxime, MiloTruck, Mohandes, NoamYakov, Picodes, RedOneN, Rohan16, Rolezn, Ruhum, RustyRabbit, Sm4rty, Soosh, StErMi, StyxRave, Tadashi, TomJ, Treasure-Seeker, TrungOre, Waze, _Adam, __141345__, ajtra, ak1, apostle0x01, arcoun, asutorufos, async, benbaessler, berndartmueller, bin2chen, brgltd, c3phas, cRat1st0s, carlitox477, chatch, codetilda, codexploder, cryptonue, cryptphi, csanuragjain, cthulhu_cult, delfin454000, dipp, dirk_y, djxploit, ellahi, exd0tpy, fatherOfBlocks, giovannidisiena, hansfriese, horsefacts, hyh, idkwhatimdoing, indijanc, jayfromthe13th, jayphbee, joestakey, kenzo, kyteg, lucacez, luckypanda, mics, minhquanym, obront, oyc_109, pedr02b2, rajatbeladiya, rbserver, reassor, robee, rokinot, rotcivegaf, sach1r0, saian, saneryee, sashik_eth, scaraven, shenwilly, simon135, sseefried, supernova, teddav, ych18, zuhaibmohd, zzzitron

Awards

167.5763 USDC - $167.58

Labels

bug

QA (Quality Assurance)

old-submission-method

External Links

Link to GitHub issue

QA

Useless fallback / receive function

Summary:

There are fallback and receive functions that does nothing. This can lead into unexpected behavior / loss funds if somebody sends ether and no withdraw / transfer flow created.

Github Permalinks:

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L459-L461 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L313-L315

Mitigation:

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)) )

Missing checks for address(0x0) when assigning values to address state variables

Summary

Zero address should be checked for state variables and some parameters in functions like mints, withdrawals... A zero address can lead into problems.

Github Permalinks

GolomTrader _governance https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L94

_distributor, distributor https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L444-L457

paymentAddress https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L67-L70

VoteEscrowDelegation https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L53 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L298-L305

GolomToken _minter, minter https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L59

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

_mint function related, should check in case no address provided https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L36-L54

RewardDistributor https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L74-L85

Mitigation

Check zero address for state variables before assigning to them a value Check in token/eth/permission assigned related the 0 address

Use of asserts()

Impact

The Solidity assert() function is meant to assert invariants. Properly functioning code should never reach a failing assert statement. A reachable assertion can mean one of two things:

A bug exists in the contract that allows it to enter an invalid state; The assert statement is used incorrectly, e.g. to validate inputs.

Details

An assertion violation was triggered. It is possible to trigger an assertion violation. Note that Solidity assert() statements should only be used to check invariants. Review the transaction trace generated for this issue and either make sure your program logic is correct, or use require() instead of assert() if your goal is to constrain user inputs or enforce preconditions. Remember to validate inputs from both callers (for instance, via passed arguments) and callees (for instance, via return values).

References

SWC ID: 110

Proof of Concept

Case 1: Exception state

SWC ID: 110 Severity: Medium Contract: VoteEscrowCore Function name: totalSupplyAt(uint256) PC address: 19762 Estimated Gas Usage: 595 - 880

In file: contracts/vote-escrow/VoteEscrowCore.sol:1206

assert(_block <= block.number)

Initial State:

Account: [CREATOR], balance: 0x504000800043, nonce:0, storage:{} Account: [ATTACKER], balance: 0x20000000000, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [ATTACKER], function: totalSupplyAt(uint256), txdata: 0x981b24d00000000000000000000000000000000000000000000000000000000000000002, value: 0x0

Case 2: Exception state

SWC ID: 110 Severity: Medium Contract: VoteEscrowCore Function name: balanceOfAtNFT(uint256,uint256) PC address: 19762 Estimated Gas Usage: 702 - 987

In file: contracts/vote-escrow/VoteEscrowCore.sol:1110

assert(_block <= block.number)

Initial State:

Account: [CREATOR], balance: 0x22000406898a1e60, nonce:0, storage:{} Account: [ATTACKER], balance: 0x1, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [ATTACKER], function: balanceOfAtNFT(uint256,uint256), txdata: 0x8c2c9baf00000000000000000000000000000000000000000000000000000000000000000280800240082008200180024000021004104080020280040801801000020002, value: 0x0

Case 3: Exception state

SWC ID: 110 Severity: Medium Contract: VoteEscrowCore Function name: setApprovalForAll(address,bool) PC address: 19762 Estimated Gas Usage: 786 - 1071

In file: contracts/vote-escrow/VoteEscrowCore.sol:666

assert(_operator != msg.sender)

Initial State:

Account: [CREATOR], balance: 0x125a20101e3c3, nonce:0, storage:{} Account: [ATTACKER], balance: 0x0, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [SOMEGUY], function: setApprovalForAll(address,bool), txdata: 0xa22cb465000000000000000000000000aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0000000000000000000000000000000000000000000000000000000000000000, value: 0x0

Case 4: Exception state

SWC ID: 110 Severity: Medium Contract: VoteEscrowCore Function name: withdraw(uint256) PC address: 19762 Estimated Gas Usage: 10323 - 33018

In file: contracts/vote-escrow/VoteEscrowCore.sol:1007

assert(_isApprovedOrOwner(msg.sender, _tokenId))

Initial State:

Account: [CREATOR], balance: 0x804110200220122a, nonce:0, storage:{} Account: [ATTACKER], balance: 0x20000000002, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [SOMEGUY], function: withdraw(uint256), txdata: 0x2e1a7d4d0008002040400000401000200000000000000000000000000000000000000000, value: 0x0

Case 5: Exception state

SWC ID: 110 Severity: Medium Contract: VoteEscrowCore Function name: increase_amount(uint256,uint256) PC address: 19762 Estimated Gas Usage: 10450 - 33145

In file: contracts/vote-escrow/VoteEscrowCore.sol:977

assert(_isApprovedOrOwner(msg.sender, _tokenId))

Initial State:

Account: [CREATOR], balance: 0x0, nonce:0, storage:{} Account: [ATTACKER], balance: 0x2, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [ATTACKER], function: increase_amount(uint256,uint256), txdata: 0xa183af5200000000000000000000000000000000008000000000000000000000000000000000800400101040020040002004804010800010008000200100020020101004, value: 0x0

Case 6: Exception state

SWC ID: 110 Severity: Medium Contract: VoteEscrowCore Function name: increase_unlock_time(uint256,uint256) PC address: 19762 Estimated Gas Usage: 10494 - 33189

In file: contracts/vote-escrow/VoteEscrowCore.sol:991

assert(_isApprovedOrOwner(msg.sender, _tokenId))

Initial State:

Account: [CREATOR], balance: 0x41000000000465, nonce:0, storage:{} Account: [ATTACKER], balance: 0x1, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [ATTACKER], function: increase_unlock_time(uint256,uint256), txdata: 0xa4d855df00040000100000000000002000100880408080020200022080020010000100000801100400040008101020000002004002800000100002000220202008001040, value: 0x0

Tools Used

Mythril, manual analysis

Github permalinks

Recommended Mitigation Steps

Consider whether the condition checked in the assert() is actually an invariant. If not, replace the assert() statement with a require() statement.

If the exception is indeed caused by unexpected behavior of the code, fix the underlying bug(s) that allow the assertion to be violated.

block.timestamp used as time proxy

Summary:

Risk of using block.timestamp for time should be considered.

Details:

block.timestamp is not an ideal proxy for time because of issues with synchronization, miner manipulation and changing block times.

This kind of issue may affect the code allowing or reverting the code before the expected deadline, modifying the normal functioning or reverting sometimes.

References

SWC ID: 116

Github Permalinks:

Some relevant timestamp usages https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol:1144 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol:1176 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol:736

Contract: VoteEscrowCore some functions where is used Function name: create_lock(uint256,uint256) Function name: checkpoint() Function name: totalSupply()

Other instances founded at contracts: https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L704

Mitigation:

Consider using an oracle for precision Consider the risk of using block.timestamp as time proxy and evaluate if block numbers can be used as an approximation for the application logic. Both have risks that need to be factored in.

Missing event for core paths/functions/parameters

Summary:

Events are important for critical parameter change and some paths of the code where ether/tokens are involved.

Details

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L98-L138 2 paths without emitting event calling add Fee doesn't return anything different nor emit an event for knowing which path of the code is followed or if some condition as (rewardToken.totalSupply() > 1000000000 * 10**18). This can cause people calling this and not knowing if they had add some fees.

Github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L98-L138
https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L141-L152
https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L155-L166
https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L171-L210
https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L215-L250

Recommended Mitigation Steps

Public external functions where money / tokens are transfered / should emit some type of event for better monitoring

Naming convention of constants

Summary:

Constant naming convention is all upper case.

Details:

Some constants are not using proper style. Constant should be in UPPER_CASE_WITH_UNDERSCORES as per Solidity Style Guide.

Github Permalinks:

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L48 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L57 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L297 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L317-L320 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L356-L357

Mitigation:

Rename the constant to uppercase style: CONSTANTS_WITH_UNDERSCORES.

Naming convention of state variable non constant

Summary:

Only constants are suggested to use style CONSTANTS_WITH_UNDERSCORES, other variables are suggested to use camelCase

Github Permalinks:

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L50 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L41

Mitigation

Rename to camelCase

Informational

Use of magic numbers is confusing and risky

Summary:

Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.

Details:

Values are hardcoded and would be more readable and maintainable if declared as a constant

I would notice that I was pretty confuse when watching for example 128 and 255 because of finding in the same contract values that comes from 2^n and 2^n-1 operations in so short of time. Documentation is not enough clear on this kind of numbers and really is hard to assume what is going on. I'm not sure, but this can be a bug of 1?

In several locations in the code numbers like 128, 255, 500, 1e18 are used. It quite easy to make a mistake somewhere, also when comparing values or missing a 0 what can lead to some bad behaviors.

Github Permalinks:

255 unexpected, is this intended to be 256? https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L745 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1167

128 unexpected, is this intended to be 127-128? https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1044 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1115

Mitigation:

Replace magic hardcoded numbers with declared constants. Define constants for the numbers used throughout the code. Comment better what this numbers are intended for

Missing indexed event parameters

Summary:

Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.

Details:

Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.

Github Permalinks:

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

Mitigation:

Consider which event parameters could be particularly useful to off-chain tools and should be indexed.

Unused code

Summary:

Code that is not used should be removed

Details:

hardhat.console.sol is being imported but non console.log in use
variable governance is declared and initialized by default to 0 but there is nothing in the code that access it. If there is something in some library that I was not able to find, take care when assigning values to it, so 0 value is not assigned.

Github Permalinks:

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L9 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L72

Mitigation:

Remove the code that is not used.

Commented out code

Summary:

There is some code that is commented out. It could point to items that are not done or need redesigning, be a mistake, or just be testing overhead.

Github Permalinks:

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L99 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L218-L225 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L6

Mitigation:

Review and remove or resolve/document the commented out lines if needed.

Use of a more recent of solidity

Summary

Rather than using abi.encodePacked for appending string, since version 0.8.12, string.concat() is enabled

Details

Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>)

Github Permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/TokenUriHelper.sol#L73-L125

Mitigation

Consider changing to pragma 0.8.12

Implementation of the comment spec confusing

Summary:

It says // ceil, avoiding overflow However, since pragma 0.8.0 overflow is by default controlled. Also there is not a unchecked operation there what would explain why the comment about overflow is there.

Github Permalinks:

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

Mitigation:

Consider removing confusing comments and or implement an unchecked scenario for gas optimization if non overflow is expected, in that case the comment would make sense.

Missing Natspec

Summary:

Missing Natspec and regular comments affect readability and maintainability of a codebase.

Details:

Contracts has partial or full lack of comments

Github Permalinks:

Some Natspec params

Missing proof and tokenId params

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L334

Natspec params

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L976-L1002 

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L91-L109  

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1225-L1236 

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L227-L238 

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L489-L551

Natspec params and return value

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L571-L580 params return

Natspec return value

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L395-L426  

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1079-L1091 

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L164

Natspec in general

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L428-L430 
https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L445-L447 

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L141-L166

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L312 

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L868-L912 

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1093-L1100 

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1155-L1158 

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1198-L1200

mitigation

Add @param descriptors Add @return descriptors Add Natspec comments. Add inline comments. Add comments for what the contract does

Duplicated interface

Summary:

IERC721 two times in VoteEscrowCore, first is IERC721, then is IERC721Metadata what also is IERC721.

Github Permalinks:

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

Mitigation:

Remove is IERC721.

Custom implementation of OZReentrancyGuard

Summary:

In this custom implementation of the well known ReentrancyGuard contract, this variables have more accessibility that needed, rather than private what would allow only the contract where the modifier is created, if a contract inherits from this one, it can modify the _entered_state variable, what is not what is expected from this.

Github Permalinks:

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L356-L358

Mitigation:

Remove internal / follow implementation of ReentrancyGuard or use OZ's library

Bad naming of modifier

Summary:

Modifiers should be mixedCase. Also the name is really similar to OZ nonReentrant but the implementation is different, what can lead to unexpected assumptions and behaviors.

Details:

Custom modifier called: nonreentrant

Github Permalinks:

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

Mitigation:

Change it to nonReentrant / or another name that is not that similar to OZ library / implement OZ nonReentrant.

So similar name in different variables or functions

Summary:

Names being so similar may lead to unexpected calls and assumptions.

Github Permalinks:

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1064 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1093 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1107 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1156

Mitigation:

Consider making more unique the names of this variables/functions

Wrong or unexpected message

Summary:

The message includes what looks like a typo and makes confusing the message.

Github Permalinks:

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

Mitigation:

Change VVDelegation to VEDelegation if that is what is expected

Bad order of code

Summary

Clearness of the code is important for the readability and maintainability. As Solidity guidelines says about declaration order: 1.Type declarations 2.State variables 3.Events 4.Modifiers 5.Functions Also, state variables order affects to gas in the same way as ordering structs for saving storage slots

Details

In this case, a constant is declared in the middle of diferent functions

github permalink

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L21-L50
Events are declared before the state variables, but also the state variables are declared without order affecting readability of the code (for example: 2 mapping variables, 1 struct, now 2 more mappings, etc). Variables of the same type should be put together.

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L21-L50
https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L15-L18
https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L44-L70
https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L284-L358
https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L41-L76

Mitigation

Follow solidity style guidelines https://docs.soliditylang.org/en/v0.8.15/style-guide.html

Warning SPDX-License missing

Summary

SPDX license should be included for avoiding compiler warnings.

Details

Compilation warnings/errors on https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol: Warning: SPDX license identifier not provided in source file. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.

github permalink

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/TokenUriHelper.sol

Mitigation

Add a SPDX License to each file

transfer as reentrancy mitigation

summary

Fixed gas cost are not good reentrancy mitigations as the cost may change by the time.

Github Permalinks:

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L203-L271

Mitigation

Avoid using transfer fixed cost as a reentrancy mitigation as the gast cost may change.

Findings Information

🌟 Selected for report: JohnSmith

Also found by: 0x1f8b, 0xA5DF, 0xDjango, 0xKitsune, 0xLovesleep, 0xNazgul, 0xSmartContract, 0xmatt, 0xsam, Aymen0909, Bnke0x0, CRYP70, Chandr, Chinmay, CodingNameKiki, Deivitto, Dravee, ElKu, Fitraldys, Funen, GalloDaSballo, Green, IllIllI, JC, Jmaxmanblue, Junnon, Kaiziron, Kenshin, Krow10, Maxime, Migue, MiloTruck, Noah3o6, NoamYakov, Randyyy, RedOneN, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, StyxRave, TomJ, Tomio, _Adam, __141345__, ajtra, ak1, apostle0x01, asutorufos, async, benbaessler, brgltd, c3phas, cRat1st0s, carlitox477, delfin454000, djxploit, durianSausage, ellahi, erictee, fatherOfBlocks, gerdusx, gogo, hyh, jayfromthe13th, jayphbee, joestakey, kaden, kenzo, kyteg, ladboy233, lucacez, m_Rassska, mics, minhquanym, oyc_109, pfapostol, rbserver, reassor, rfa, robee, rokinot, sach1r0, saian, samruna, sashik_eth, simon135, supernova, tofunmi, zuhaibmohd

Awards

94.6571 USDC - $94.66

Labels

bug

G (Gas Optimization)

old-submission-method

External Links

Link to GitHub issue

GAS

Public function visibility can be made external

summary

Functions should have the strictest visibility possible. Public functions may lead to more gas usage by forcing the copy of their parameters to memory from calldata.

details

If a function is never called from the contract it should be marked as external. This will save gas.

github permalinks

mitigation

Consider changing visibility from public to external.

use of custom errors rather than revert() / require()

summary

Custom errors reduce 38 gas if the condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.

details

Since version 0.8.4 the use of custom errors rather than revert() / require() saves gas as noticed in https://blog.soliditylang.org/2021/04/21/custom-errors/ https://github.com/code-423n4/2022-04-pooltogether-findings/issues/13

github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L173 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L181 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L184 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L185 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L220 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L292 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L309

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L72-L73 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L99 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L130 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L186 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L211 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L239

mitigation

replace each error message in a require by a custom error

duplicated require() check should be refactored

summary

duplicated require() / revert() checks should be refactored to a modifier or function to save gas

details

Event appears twice and can be reduced

github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L235 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L299 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L359

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L295 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L349

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L227 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L296 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L350

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L43 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L51

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L292 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L309

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L130 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L186

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L946 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L999

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L929 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L983

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

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L144 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L158

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L226 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L295 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L349

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L869 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L874 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L879 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L884 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L889

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L927 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L944 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L981

mitigation

refactor this checks to different functions to save gas

use != rather than >0 for unsigned integers in require() statements

Summary

When the optimizer is enabled, gas is wasted by doing a greater-than operation, rather than a not-equals operation inside require() statements. When Using != , the optimizer is able to avoid the EQ, ISZERO, and associated operations, by relying on the JUMPI that comes afterwards, which itself checks for zero.

github permalinks

mitigation

Use != 0 rather than > 0 for unsigned integers in require() statements.

splitting require() statements that use && saves gas

Summary

github permalinks

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

mitigation

Instead of using the && operator in a single require statement to check multiple conditions. Consider using multiple require statements with 1 condition per require statement (saving 3 gas per & ):

Reduce the size of error messages (Long revert Strings)

summary

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.

github permalinks

mitigation

Consider shortening the revert strings to fit in 32 bytes

usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

summary

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

details

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size than downcast where needed

github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L356-L358 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L320

mitigation

Consider using some data type that uses 32 bytes, for example uint256

Using bools for storage incurs overhead {

summary

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

details

Here is one example of OpenZeppelin about this optimization https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L20-L21 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L55 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L439-L441 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L650-L651 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L314 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L341-L344

mitigation

Consider using uint256 with values 0 and 1 rather than bool

Pack structs tightly

summary

Gas efficiency can be achieved by tightly packing the struct. Struct variables are stored in 32 bytes each so you can group smaller types to occupy less storage. For example, startedAt or boughtAt in Auction struct hold block.number so realistically this does not need uint256 and you can consider storing it in lower type.

details

You can read more here: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html or in the official documentation: https://docs.soliditylang.org/en/v0.4.21/miscellaneous.html

github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L47-L65

mitigation

Search for an optimal size and order of structs to reduce gas usage.

Store using Struct over multiple mappings

summary

All these variables could be combine in a Struct in order to reduce the gas cost.

details

As noticed in: https://gist.github.com/alexon1234/b101e3ac51bea3cbd9cf06f80eaa5bc2 When multiple mappings that access the same addresses, uints, etc, all of them can be mixed into an struct and then that data accesed like: mapping(datatype => newStructCreated) newStructMap; Also, you have this post where it explains the benefits of using Structs over mappings https://medium.com/@novablitz/storing-structs-is-costing-you-gas-774da988895e

github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L332 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L335 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L341

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L338 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L329 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L326 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L307-L314 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L304 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L302

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L31-L35 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L43-L47

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L58-L59

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L60-L67 I think all of the uint256 key => are the same also in this case (epoch)

mitigation

Consider mixing different mappings into an struct when able in order to save gas.

Make constant state variables that do not change

summary

State variables which value isn't changed by any function in the contract, can be declared as a constant state variable to save some gas during deployment.

github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L72

mitigation

Add constant to state variables that do not change
Add immutable to state variables that do not change but which value is assigned in constructor

Using private rather than public for constants saves gas

summary

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L317-L320

mitigation

Consider replacing public for private in constants for gas saving.

Explicit initialization

summary

It is not needed to initialize variables to the default value. Explicitly initializing it with its default value is an anti-pattern and wastes gas.

details

If a variable is not set/initialized, it is assumed to have the default value ( 0 for uint, false for bool, address(0) for address…).

github permalinks

mitigation

Don't initialize variables to default value

Index initialized in for loop

summary

In for loops is not needed to initialize indexes to 0 as it is the default uint/int value. This saves gas.

github permalinks

mitigation

Don't initialize variables to default value

use of i++ in loop rather than ++i

summary

++i costs less gas than i++, especially when it's used in for loops

details

using ++i doesn't affect the flow of regular for loops and improves gas cost

github permalinks

mitigation

Substitute to ++i

++i costs less gas compared to i++ or i+=1, the same with happens with --i and i-- or i-=1

summary

++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.

details

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

github permalinks

+= 1 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L499 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L768 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L118 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L885

-= 1 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/TokenUriHelper.sol#L143 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L512 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L890

var++ https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/TokenUriHelper.sol#L138

mitigation

Replace to ++i or --i as needed.

increments can be unchecked in loops

summary

Unchecked operations as the ++i on for loops are cheaper than checked one.

details

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline..

The code would go from:

for (uint256 i; i < numIterations; i++) {
// ...
}

to

for (uint256 i; i < numIterations;) {
  // ...
  unchecked { ++i; }
}
The risk of overflow is inexistent for a uint256 here.

github permalinks

mitigation

Add unchecked ++i at the end of all the for loop where it's not expected to overflow and remove them from the for header

<array>.length should no be looked up in every loop of a for-loop

summary

In loops not assigning the length to a variable so memory accessed a lot (caching local variables)

details

The overheads outlined below are PER LOOP, excluding the first loop storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas)

github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L415

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L143 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L157 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L180 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L186

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L171 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L189 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L199

mitigation

Assign the length of the array.length to a local variable in loops for gas savings

Variables should be cached when used several times

summary

loop length variables assigned to a local variable improves gas usage

details

In loops or state variables, this is even more gas saving

github permalinks

epochs[index] https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L144-L149 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L158-L163

epochs[index], epochBeginTime[epochs[index]], tokenids[tindex] https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L184-L204

epochBeginTime[index] https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L237-L246

mitigation

Cache variables used more than one into a local variable.

Shift right instead of dividing by 2

Summary

Shifting is cheaper than dividing by 2

Details

A division by 2 can be calculated by shifting one to the right. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity’s division operation also includes a division-by-0 prevention which is bypassed using shifting.

Github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1049 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1120 https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L150

mitigation

Consider replacing / 2 with >> 1 here

Internal functions only called once can be inlined to save gas

Summary

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Github permalinks

https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L111-L161 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L452-L457 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L462-L487

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L570-L580 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1107-L1154

mitigation

Consider changing internal function only called once to inline code for gas savings

abi.encode() is more efficient than abi.encodePacked()

Summary

In general, abi.encodePacked is more gas-efficient.

Details

Changing the abi.encode function to abi.encodePacked can save gas since the abi.encode function pads extra null bytes at the end of the call data, which is unnecessary. Also, in general, abi.encodePacked is more gas-efficient.

Github permalinks

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L102-L108 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L115-L119 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L130-L147

mitigation

Recommended Mitigation Steps Change abi.encode to abi.encodePacked at line 355.

Functions guaranteed to revert when called by normal users can be marked payable

Summary

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function.

Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

Details

The extra opcodes avoided are: CALLVALUE (2), DUP1 (3), ISZERO (3), PUSH2 (3), JUMPI (10), PUSH1 (3), DUP1 (3), REVERT(0), JUMPDEST (1), POP (2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

Github permalinks

Mitigation

It's suggested to add payable to functions guaranteed to revert when called by normal users to improve gas costs

>= cheaper than >

Summary

Strict inequalities ( > ) are more expensive than non-strict ones ( >= ). This is due to some supplementary checks (ISZERO, 3 gas)

Github permalinks

mitigation

Consider using >= 1 instead of > 0 to avoid some opcodes

<X> += <Y> costs more gas than <X> = <X> + <Y> for state variables

Summary

x+=y costs more gas than x=x+y for state variables

Github permalinks

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

Mitigation

Don't use += for state variables as it cost more gas.

Golom contest - Deivitto's results

An NFT marketplace that offers the lowest industry fee, a publicly available order-book along with analytical tools.

General Information

Findings Distribution

Researcher Performance

External Links

[M-01] Use of transfer() instead of call() to send eth - $0.00

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Use of transfer() instead of call() to send eth

Impact

References

Proof of Concept

Recommended Mitigation Steps

#0 - KenzoAgada2022-08-03T14:02:40Z

[Q-47] QA Report - $167.58

Findings Information

Awards

Labels

External Links

QA

Useless fallback / receive function

Summary:

Github Permalinks:

Mitigation:

Missing checks for address(0x0) when assigning values to address state variables

Summary

Github Permalinks

Mitigation

Use of asserts()

Impact

Details

References

Proof of Concept

Case 1: Exception state

Case 2: Exception state

Case 3: Exception state

Case 4: Exception state

Case 5: Exception state

Case 6: Exception state

Tools Used

Github permalinks

Recommended Mitigation Steps

block.timestamp used as time proxy

Summary:

Details:

References

Github Permalinks:

Mitigation:

Missing event for core paths/functions/parameters

Summary:

Details

Github permalinks

Recommended Mitigation Steps

Naming convention of constants

Summary:

Details:

Github Permalinks:

Mitigation:

Naming convention of state variable non constant

Summary:

Github Permalinks:

Mitigation

Informational

Use of magic numbers is confusing and risky

Summary:

Details:

Github Permalinks:

Mitigation:

Missing indexed event parameters

Summary:

Details:

Github Permalinks:

Mitigation:

Unused code

Summary:

#0 - KenzoAgada
2022-08-03T14:02:40Z