Golom contest - ellahi's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L151-L156

Vulnerability details

Impact

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

The claimer smart contract does not implement a payable function. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit. 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. Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Proof of Concept

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L151-L156

function payEther(uint256 payAmt, address payAddress) internal { // @audit transfer => call
        if (payAmt > 0) {
            // if royalty has to be paid
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }
    }

Tools Used

Manual.

Recommended Mitigation Steps

I recommend using call() instead of transfer()

Lines of code

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

Vulnerability details

Impact

According to Openzeppelin's ERC721.sol::_safeTransfer()

* @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients
* are aware of the ERC721 protocol to prevent tokens from being forever locked.

Proof of Concept

  GolomTrader.sol::236 => ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
  GolomTrader.sol::301 => nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);
  GolomTrader.sol::361 => nftcontract.transferFrom(msg.sender, o.signer, tokenId);

Tools Used

Manual.

Recommended Mitigation Steps

Use OpenZeppelin's safeTransferFrom to prevent NFT's from being lost.

Low

[L-01] Incorrect startTime

Impact

The RewardDistributor contract uses a hardcoded startTime which starts before the audit ends.

Proof of Concept

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

        startTime = 1659211200; // @audit starts july 30th but audit ends aug 1st

Recommendation

Use the intended start time.

[L-02] Unsafe ERC20 Operations

Impact

The return value of an external transfer/transferFrom call is not checked

Proof of Concept

  GolomTrader.sol::382 => WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);
  RewardDistributor.sol::151 => rewardToken.transfer(addr, reward);
  RewardDistributor.sol::165 => rewardToken.transfer(addr, reward);
  RewardDistributor.sol::208 => rewardToken.transfer(tokenowner, reward);
  RewardDistributor.sol::209 => weth.transfer(tokenowner, rewardEth);
  VoteEscrowCore.sol::861 => assert(IERC20(token).transferFrom(from, address(this), _value));
  VoteEscrowCore.sol::1023 => assert(IERC20(token).transfer(msg.sender, value));

Recommendation

Use SafeERC20, or ensure that the transfer/transferFrom return value is checked.

[L-03] Unspecific Compiler Version Pragma

Impact

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

Proof of Concept

  GolomToken.sol::2 => pragma solidity ^0.8.11;
  GovernerBravo.sol::2 => pragma solidity ^0.8.11;

Recommendation

Avoid floating pragmas for non-library contracts. It is recommended to pin to a concrete compiler version.

Non-Critical

[N-01] Typos

Timlock.sol => Timelock.sol GovernerBravo.sol => GovernorBravo.sol GovernerBravo.sol but the contract name is GovernorAlpha

Tools used

manual, slither

Gas

[G-01] Remove import hardhat/console.sol before deployment

Impact

Waste of gas.

Proof of Concept

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

import 'hardhat/console.sol';

Recommendation

Remove the import.

[G-02] Cache Array Length Outside of Loop.

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

  GolomTrader.sol::415 => for (uint256 i = 0; i < proof.length; i++) {
  GovernerBravo.sol::240 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  GovernerBravo.sol::269 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  GovernerBravo.sol::293 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  RewardDistributor.sol::143 => for (uint256 index = 0; index < epochs.length; index++) {
  RewardDistributor.sol::157 => for (uint256 index = 0; index < epochs.length; index++) {
  RewardDistributor.sol::180 => for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
  RewardDistributor.sol::183 => for (uint256 index = 0; index < epochs.length; index++) {

Recommendation

Store the array’s length in a variable before the for-loop.

[G-03] Use != 0 instead of > 0 for Unsigned Integer Comparison in require statements.

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.

Proof of Concept

  GovernerBravo.sol::325 => require(proposalCount >= proposalId && proposalId > 0, 'GovernorAlpha::state: invalid proposal id');
  VoteEscrowCore.sol::927 => require(_value > 0); // dev: need non-zero value
  VoteEscrowCore.sol::944 => require(_value > 0); // dev: need non-zero value

Recommendation

Use != 0 instead of > 0.

[G-04] Use Custom Errors instead of Revert Strings.

Impact

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

Recommendation

Use custom errors instead of revert strings.

[G-05] No need to initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

  GolomTrader.sol::415 => for (uint256 i = 0; i < proof.length; i++) {
  GovernerBravo.sol::240 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  GovernerBravo.sol::269 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  GovernerBravo.sol::293 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  RewardDistributor.sol::142 => uint256 reward = 0;
  RewardDistributor.sol::143 => for (uint256 index = 0; index < epochs.length; index++) {
  RewardDistributor.sol::156 => uint256 reward = 0;
  RewardDistributor.sol::157 => for (uint256 index = 0; index < epochs.length; index++) {
  RewardDistributor.sol::175 => uint256 reward = 0;
  RewardDistributor.sol::176 => uint256 rewardEth = 0;
  RewardDistributor.sol::180 => for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
  RewardDistributor.sol::183 => for (uint256 index = 0; index < epochs.length; index++) {
  RewardDistributor.sol::222 => uint256 reward = 0;
  RewardDistributor.sol::223 => uint256 rewardEth = 0;
  RewardDistributor.sol::226 => for (uint256 index = 0; index < epoch; index++) {
  RewardDistributor.sol::257 => uint256 reward = 0;
  RewardDistributor.sol::258 => for (uint256 index = 0; index < epoch; index++) {
  RewardDistributor.sol::272 => uint256 reward = 0;
  RewardDistributor.sol::273 => for (uint256 index = 0; index < epoch; index++) {
  VoteEscrowCore.sol::745 => for (uint256 i = 0; i < 255; ++i) {
  VoteEscrowCore.sol::1044 => for (uint256 i = 0; i < 128; ++i) {
  VoteEscrowCore.sol::1115 => for (uint256 i = 0; i < 128; ++i) {
  VoteEscrowCore.sol::1167 => for (uint256 i = 0; i < 255; ++i) {
  VoteEscrowCore.sol::1211 => uint256 dt = 0;

Recommendation

Remove explicit default initializations.

[G-06] ++i costs less gas compared to i++ or i += 1

Impact

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

Proof of Concept

  GolomTrader.sol::415 => for (uint256 i = 0; i < proof.length; i++) {
  GovernerBravo.sol::240 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  GovernerBravo.sol::269 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  GovernerBravo.sol::293 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  RewardDistributor.sol::143 => for (uint256 index = 0; index < epochs.length; index++) {
  RewardDistributor.sol::157 => for (uint256 index = 0; index < epochs.length; index++) {
  RewardDistributor.sol::180 => for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
  RewardDistributor.sol::183 => for (uint256 index = 0; index < epochs.length; index++) {

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.

[G-07] Use Shift Right/Left instead of Division/Multiplication whenever possible

Impact

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left. 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.

Proof of Concept

  VoteEscrowCore.sol::1049 => uint256 _mid = (_min + _max + 1) / 2;
  VoteEscrowCore.sol::1120 => uint256 _mid = (_min + _max + 1) / 2;

Recommendation

Use SHR/SHL. Bad

uint256 b = a / 2;
uint256 c = a / 4;
uint256 d = a * 8;

Good

uint256 b = a >> 1;
uint256 c = a >> 2;
uint256 d = a << 3;

Tools used

c4udit, manual, slither