Golom contest - RedOneN's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L154 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L245 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L248 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L251 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L252 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L262 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L267 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L384 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L385 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L386 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L388 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L389 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L396 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L401

Vulnerability details

Impact

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when: 1) The claimer smart contract does not implement a payable function. 2) The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit. 3) The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300.

Proof of Concept

See link to affected code.

Tools Used

manual audit

Recommended Mitigation Steps

I recommend using call() instead of transfer().

Lines of code

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

Vulnerability details

Impact

In GolomTrader.sol the fillAsk(), fillBid() and fillCriteriaBid() functions call transferFrom() on a ERC721 token. This does not ensure that the token is not sent to an address that is not able to properly support it which could result in the loss of the token.

Note as well that openzepellin’s documentation discourages the use of transferFrom. Indeed, it is highly suggested to use safeTransferFrom() whenever possible.

Proof of Concept

See links to affected code

Tools Used

Manual audit

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom(). Note that the contract should inherit the ERC721TokenReceiver contract as a consequence.

[L01] Unused/Empty RECEIVE()/FALLBACK() Function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert.

File: RewardDistributor.sol RewardDistributor.sol#L313 RewardDistributor.sol#L315

File: GolomTrader.sol GolomTrader.sol#L459 GolomTrader.sol#L461

[N01] Consider declaring and using constant instead of magic numbers

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L99

File: RewardDistributor.sol RewardDistributor.sol#L100 RewardDistributor.sol#L120 RewardDistributor.sol#L121

[N02] Description of functions should not be a generic message

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L68

[N03] REQUIRE statement should have an error message in case of REVERT

File: RewardDistributor.sol RewardDistributor.sol#L144 RewardDistributor.sol#L158

File: GolomTrader.sol GolomTrader.sol#L220 GolomTrader.sol#L231 GolomTrader.sol#L285 GolomTrader.sol#L291 GolomTrader.sol#L293 GolomTrader.sol#L295 GolomTrader.sol#L231 GolomTrader.sol#L311 GolomTrader.sol#L342 GolomTrader.sol#L345 GolomTrader.sol#L347 GolomTrader.sol#L349 GolomTrader.sol#L350

File: VoteEscrowCore.sol VoteEscrowCore.sol#L540

[N04] Consider Using the latest version of solidity

[G01] Use custom errors rather than revert()/require() strings

Custom errors are ABI encoded. Consequently, they can be decoded using ABI decoders. This makes it more gas efficient than « revert string ».

File: GolemToken.sol GolomToken.sol#L24 GolomToken.sol#L43 GolomToken.sol#L51 GolomToken.sol#L69

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L72 VoteEscrowDelegation.sol#L73 VoteEscrowDelegation.sol#L99 VoteEscrowDelegation.sol#L130 VoteEscrowDelegation.sol#L186 VoteEscrowDelegation.sol#L211 VoteEscrowDelegation.sol#L239 VoteEscrowDelegation.sol#L245

File: RewardDistributor.sol RewardDistributor.sol#L173 RewardDistributor.sol#L181 RewardDistributor.sol#L184 RewardDistributor.sol#L185 RewardDistributor.sol#L220

File: GolomTrader.sol GolomTrader.sol#L177 GolomTrader.sol#L211 GolomTrader.sol#L217 GolomTrader.sol#L220 GolomTrader.sol#L222 GolomTrader.sol#L226 GolomTrader.sol#L227 GolomTrader.sol#L235 GolomTrader.sol#L299 GolomTrader.sol#L359 GolomTrader.sol#L455

File: VoteEscrowCore.sol VoteEscrowCore.sol#L538

[G02] require()/revert() strings longer than 32 bytes cost extra gas

File: GolemToken.sol GolomToken.sol#L24

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L73

File: RewardDistributor.sol RewardDistributor.sol#L292 RewardDistributor.sol#L310

File: VoteEscrowCore.sol VoteEscrowCore.sol#L945 VoteEscrowCore.sol#L946 VoteEscrowCore.sol#L983

[G03] Functions guaranteed to revert when called by normal users (such as onlyOwner) can be marked payable.

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.

File: GolemToken.sol GolomToken.sol#L36 GolomToken.sol#L42 GolomToken.sol#L50 GolomToken.sol#L58 GolomToken.sol#L65

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L260

File: RewardDistributor.sol RewardDistributor.sol#L98 RewardDistributor.sol#L285 RewardDistributor.sol#L291 RewardDistributor.sol#L308

File: GolomTrader.sol GolomTrader.sol#L444 GolomTrader.sol#L454

[G04] It costs more gas to initialize variables to zero than to let the default of zero be applied.

This is mainly the case for storage but is also observed for memory.

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L50 VoteEscrowDelegation.sol#L147 VoteEscrowDelegation.sol#L170 VoteEscrowDelegation.sol#L171 VoteEscrowDelegation.sol#L188 VoteEscrowDelegation.sol#L189

File: RewardDistributor.sol RewardDistributor.sol#L45 RewardDistributor.sol#L142 RewardDistributor.sol#L143 RewardDistributor.sol#L156 RewardDistributor.sol#L157 RewardDistributor.sol#L175 RewardDistributor.sol#L176 RewardDistributor.sol#L180 RewardDistributor.sol#L183 RewardDistributor.sol#L222 RewardDistributor.sol#L223 RewardDistributor.sol#L226 RewardDistributor.sol#L272 RewardDistributor.sol#L273

File : GolomTrader GolomTrader.sol#L415

[G05] Pre-increments (++i) cost less gas than post-increment (i++), especially inside for loops.

Saves 6 gas per loop 

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L171 VoteEscrowDelegation.sol#L188 VoteEscrowDelegation.sol#L189 VoteEscrowDelegation.sol#L199

File: RewardDistributor.sol RewardDistributor.sol#L143 RewardDistributor.sol#L157 RewardDistributor.sol#L180 RewardDistributor.sol#L183 RewardDistributor.sol#L226 RewardDistributor.sol#L273

File : GolomTrader GolomTrader.sol#L415

[G06] ++i should be unchecked{++I} when it is not possible for them to overflow (ie. In a for loop)

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L171 VoteEscrowDelegation.sol#L189 VoteEscrowDelegation.sol#L199

File: RewardDistributor.sol RewardDistributor.sol#L143 RewardDistributor.sol#L157 RewardDistributor.sol#L180 RewardDistributor.sol#L183 RewardDistributor.sol#L226 RewardDistributor.sol#L273

File : GolomTrader GolomTrader.sol#L415

[G07] Using >0 costs more gas than != when used on a UINT

This is especially true inside REQUIRE statement but also applies in IF statement.

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L78 VoteEscrowDelegation.sol#L103 VoteEscrowDelegation.sol#L119

File: VoteEscrowCore.sol VoteEscrowCore.sol#L981 VoteEscrowCore.sol#L982 VoteEscrowCore.sol#L996 VoteEscrowCore.sol#L997

[G08] For memory variable "x"+="y" is more gas efficient than "x"="x"+"y"

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L172 VoteEscrowDelegation.sol#L190

File: RewardDistributor.sol RewardDistributor.sol#L230 RewardDistributor.sol#L238 RewardDistributor.sol#L241 RewardDistributor.sol#L274

[G09] External functions are less expensive than public

Some functions are set to public although they are not called within the contract. Consider turning those that are only called outside the contract as « external » to save gas.

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L185

File: GolomTrader.sol GolomTrader.sol#L203 —> function FillAsk() GolomTrader.sol#L279 —> function FillBid() GolomTrader.sol#L312 —> function CancelOrder() GolomTrader.sol#L334 —> function FillCriteriaBid()

[G10] REQUIRE statement should be placed at the beginning of a function to save gas.

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L245 —> should come before line 242

[G11] Splitting require() statements that use « && » saves gas

File: VoteEscrowDelegation.sol VoteEscrowDelegation.sol#L239

File: VoteEscrowCore.sol VoteEscrowCore.sol#L538

[G12] Duplicated Require()/Revert() checks should be refactored to a modifier or function

File: VoteEscrowDelegation.sol « VEDelegation: Not allowed » appears twice VoteEscrowDelegation.sol#L72 VoteEscrowDelegation.sol#L211 « VEDelegation: not yet determined » appears twice VoteEscrowDelegation.sol#L130 VoteEscrowDelegation.sol#L186

File: RewardDistributor.sol « RewardDistributor: time not over yet » appears twice RewardDistributor.sol#L292 RewardDistributor.sol#L309

[G13] Variable that are only set in the constructor should be declared as immutable

File: RewardDistributor.sol RewardDistributor.sol#L43

[G14] Caching storage variable in memory to save gas

File: RewardDistributor.sol Variable: epoch (2sloads) RewardDistributor.sol#L106 RewardDistributor.sol#L118 Variable: rewardsToken.totalSupply() (4sloads) RewardDistributor.sol#L100 RewardDistributor.sol#L112 RewardDistributor.sol#L113 RewardDistributor.sol#L114 Variable: rewardsToken.balanceOf(address(ve)) (2sloads) RewardDistributor.sol#L112 RewardDistributor.sol#L114 Variable: epoch (N sloads since in a for loop) RewardDistributor.sol#L226

[G15] Consider loading variable that are already in memory rather than reloading from storage

File: RewardDistributor.sol Function: StakerRewards, RewardDistributor.sol#L228 —> could use « unclaimeddepochs » (memory defined at line 227) instead of « claimed »

[G16] Usage of Uints/Ints smaller than 256bits incurs overhead

EVM operates on a 32bytes basis. If an element is smaller than 32bytes, the EVM has to perform operation in order to get the desired size. As a result, defining elements smaller than 32bytes might result in higher gas cost. It is better to work with 256 bits and downcast where needed.

File: GolomTrader.sol GolomTrader.sol#L62

File: VoteEscrowCore.sol VoteEscrowCore.sol#L261 VoteEscrowCore.sol#L262 VoteEscrowCore.sol#L297 VoteEscrowCore.sol#L356 VoteEscrowCore.sol#L357 VoteEscrowCore.sol#L358

[G17] Consider variable packing to save gas

File: GolomTrader.sol GolomTrader.sol#L62 —> « uint8 v » should be placed after an address variable.

[G18] Consider saving the 50basis points fee in memory to avoid recompilation and save gas.

File: GolomTrader.sol Function : fillAsk() GolomTrader.sol#L212 GolomTrader.sol#L242 GolomTrader.sol#L254 GolomTrader.sol#L263 GolomTrader.sol#L269

[G19] Consider using the pre-increment « ++var » instead of « var +=1 » to save gas.

File: VoteEscrowCore.sol VoteEscrowCore.sol#L499 VoteEscrowCore.sol#L512

[G20] Require() should be used instead of assert() to save gas left during a transaction.

File: VoteEscrowCore.sol VoteEscrowCore.sol#L493 VoteEscrowCore.sol#L506 VoteEscrowCore.sol#L519 VoteEscrowCore.sol#L666 VoteEscrowCore.sol#L679