Golom contest - asutorufos's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#:~:text=payable(payAddress).transfer(payAmt)%3B%20//%20royalty%20transfer%20to%20royaltyaddress#L154

Vulnerability details

Impact

When transferring ETH, use call() instead of transfer().

The transfer() function only allows the recipient to use 2300 gas. If the recipient uses more than that, transfers will fail. In the future gas costs might change increasing the likelihood of that happening.

Proof of Concept

GolomTrader.sol L#154

Recommended Mitigation Steps

Replacing transfer with call

(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed.")

L-1 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 (e.g. require(msg.sender == address(weth)) ) RewardDistributor.sol L#313-315

GolomTrader.sol L#459-461

L-2 MISSING CHECKS FOR ADDRESS(0X0) WHEN ASSIGNING VALUES TO ADDRESS STATE VARIABLES GolomToken.sol L#59 RewardDistriubtor.sol L#287

N-1 LARGE MULTIPLES OF TEN SHOULD USE SCIENTIFIC NOTATION (E.G. 1E9) RATHER THAN DECIMAL LITERALS (E.G. 1000000000), FOR READABILITY RewardDistributor.sol L#100

N-2 Require()/Revert() STATEMENTS SHOULD HAVE DESCRIPTIVBE REASON STRINGS

File: contracts/rewards/RewardDistributor.sol

144            require(epochs[index] < epoch);


158            require(epochs[index] < epoch);

G-1 MULTIPLE ADDRESS MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS TO A STRUCT, WHERE APPROPRIATE Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.

File: contracts/core/VoteEscrowCore.sol
 
   331 /// @dev Mapping from owner address to count of his tokens.
   332 mapping(address => uint256) internal ownerToNFTokenCount;

   334  /// @dev Mapping from owner address to mapping of index to tokenIds
   335 mapping(address => mapping(uint256 => uint256)) internal ownerToNFTokenIdList;

   337 /// @dev Mapping from NFT ID to index of owner
   338 mapping(uint256 => uint256) internal tokenToOwnerIndex;

   340 /// @dev Mapping from owner address to mapping of operator addresses.
   341 mapping(address => mapping(address => bool)) internal ownerToOperators;

G-2 State Variables only set in the constructor should be declared imutable Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas). RewardDistributor.sol L#44

G-3 <ARRAY>.LENGTH SHOULD NOT BE LOOKED UP IN EVERY LOOP OF A FOR-LOOP 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)

VoteEscrowDelegation.sol L#171

VoteEscrowDelegation.sol L#189

VoteEscrowDelegation.sol L#199

G-4 USING > 0 COST MORE GAS THAN != 0 WHEN USED ON A UINT IN A REQUIRE() STATEMENT VoteEscrowCore.sol L#927-928%3B,-require(_locked.end)

[VoteEscrowCore.sol L#944]https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#:~:text=down%20to%20weeks-,require(_value%20%3E%200)%3B%20//%20dev%3A%20need%20non%2Dzero%20value,-require(unlock_time%20%3E

VoteEscrowCore.sol L#997