Golom contest - brgltd's results

Lines of code

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

Vulnerability details

Impact

Using transfer will forward 2300 gas. A smart contract receiving the ether can run out of gas, which would make the payment impossible. Using call will forward all the gas.

154: payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress

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

Proof of Concept

1. Ether is transferred for a contract that will consume more than the 2300 gas.
2. The transaction will fail.

Recommended Mitigation Steps

Replace transfer with call. Note that the return of call needs to be checked.

$ git diff --no-index GolomTrader.sol.orig GolomTrader.sol
diff --git a/GolomTrader.sol.orig b/GolomTrader.sol
index 70a18ad..d8cd7aa 100644
--- a/GolomTrader.sol.orig
+++ b/GolomTrader.sol
@@ -151,7 +151,8 @@ contract GolomTrader is Ownable, ReentrancyGuard {
     function payEther(uint256 payAmt, address payAddress) internal {
         if (payAmt > 0) {
             // if royalty has to be paid
-            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
+            (bool sent, ) = payAddress.call{value: payAmt}(""); // royalty transfer to royaltyaddress
+            require(sent);
         }
     }

Lines of code

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

Vulnerability details

Impact

OpenZeppelin recommends using safeTransferFrom instead of transferFrom.

Using safeTransferFrom is also recommended since some NFTs have logic in the onERC721Received function which is only triggered with safeTransferFrom. Example

Proof of Concept

1. ERC721 transfer is executed with transferFrom.
2. transferFrom will not check if the contract recipients are aware of the ERC721 protocol to prevent tokens from being locked forever.

Recommended Mitigation Steps

The instances bellow should make of the safeTransferFrom from OpenZeppelin instead of transferFrom.

File:  contracts/core/GolomTrader.sol
236: ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

301: nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);

361: nftcontract.transferFrom(msg.sender, o.signer, tokenId);

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

[L-01] Remove floating pragma

Locking the pragma will make sure that the contract does not get deployed using outdated compiler versions.

File:
2: pragma solidity ^0.8.11;

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol

[NC-01] Remove comments from previous debug tests

Comments with console.logs should be removed from the codebase.

File: contracts/rewards/RewardDistributor.sol
99: //console.log(block.timestamp,epoch,fee);

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

[NC-02] Avoid returning a tuple if a function returns only one item

File: contracts/rewards/RewardDistributor.sol
function traderRewards(address addr) public view returns (
        uint256
        ){
    uint256 reward = 0;
    for (uint256 index = 0; index < epoch; index++) {
        reward =
            reward +
            (rewardTrader[index] * feesTrader[addr][index]) /
            epochTotalFee[index];
    }
    return (reward);
}

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

[N-03] Inconsistent spacing for else keyword

Add a spacing before and after the else keyword

File: contracts/rewards/RewardDistributor.sol#L194
194: }else{

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

[N-04] Remove unused state variables

State variables that are not used inside the contract should be removed.

File: contracts/core/GolomTrader.sol
72: address public governance;

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

[N-05] Convert public functions not consumed by the contract to external

File: contracts/core/GolomTrader.sol
279: function fillBid(

312: function cancelOrder(Order calldata o) public nonReentrant {

334: function fillCriteriaBid(

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

File: contracts/rewards/RewardDistributor.sol
98: function addFee(address[2] memory addr, uint256 fee) public onlyTrader {

141: function traderClaim(address addr, uint256[] memory epochs) public {

155: function exchangeClaim(address addr, uint256[] memory epochs) public {

172: function multiStakerClaim(uint256[] memory tokenids, uint256[] memory epochs) public {

215: function stakerRewards(uint256 tokenid) public view returns (

254: function traderRewards(address addr) public view returns (

296: function exchangeRewards(address addr) public view returns (

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

[G-01] ++i costs less gas than i++, especially in for loops

Saves about 5 gas per loop.

File: contracts/core/GolomTrader.soI
415: for (uint256 i = 0; i < proof.length; i++) {

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

File: contracts/rewards/RewardDistributor.sol
143: for (uint256 index = 0; index < epochs.length; index++) {

157: for (uint256 index = 0; index < epochs.length; index++) {

180: for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {

183: for (uint256 index = 0; index < epochs.length; index++) {

226: for (uint256 index = 0; index < epoch; index++) {

258: for (uint256 index = 0; index < epoch; index++) {

273: for (uint256 index = 0; index < epoch; index++) {

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

File: contracts/vote-escrow/TokenUriHelper.sol
138: digits++;

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

File: contracts/vote-escrow/VoteEscrowDelegation.sol
171: for (uint256 index = 0; index < delegated.length; index++) {

189: for (uint256 index = 0; index < delegatednft.length; index++) {

199: for (uint256 i; i < _array.length; i++) {

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol

[G-02] The increment in for loop post condition can be made unchecked

The solidity compiler will apply arithmetic checks for the increment step during for loops. This can be disabled since the value being incremented won't surpass the upper bound that's checked on the break condition.

Adding uncheck can save 30-40 gas per loop.

File: contracts/core/GolomTrader.soI
415: for (uint256 i = 0; i < proof.length; i++) {

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

File: contracts/rewards/RewardDistributor.sol
143: for (uint256 index = 0; index < epochs.length; index++) {

157: for (uint256 index = 0; index < epochs.length; index++) {

180: for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {

183: for (uint256 index = 0; index < epochs.length; index++) {

226: for (uint256 index = 0; index < epoch; index++) {

258: for (uint256 index = 0; index < epoch; index++) {

273: for (uint256 index = 0; index < epoch; index++) {

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

File: contracts/vote-escrow/VoteEscrowDelegation.sol
171: for (uint256 index = 0; index < delegated.length; index++) {

189: for (uint256 index = 0; index < delegatednft.length; index++) {

199: for (uint256 i; i < _array.length; i++) {

File: contracts/vote-escrow/VoteEscrowCore.sol
1044: for (uint256 i = 0; i < 128; ++i) {

1115: for (uint256 i = 0; i < 128; ++i) {

1167: for (uint256 i = 0; i < 255; ++i) {

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol

[G-03] Array.length should not be computed on every interation during a loop

Instead of computing array.length for every iteration, the value for array.length should be cached before the loop to save gas.

File: contracts/core/GolomTrader.soI
415: for (uint256 i = 0; i < proof.length; i++) {

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

File: contracts/rewards/RewardDistributor.sol
143: for (uint256 index = 0; index < epochs.length; index++) {

157: for (uint256 index = 0; index < epochs.length; index++) {

180: for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {

183: for (uint256 index = 0; index < epochs.length; index++) {

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

File: contracts/vote-escrow/VoteEscrowDelegation.sol
171: for (uint256 index = 0; index < delegated.length; index++) {

189: for (uint256 index = 0; index < delegatednft.length; index++) {

199: for (uint256 i; i < _array.length; i++) {

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol

[G-04] It costs more gas to initialize variables to zero than to let the default of zero be applied

File: contracts/core/GolomTrader.soI
415: for (uint256 i = 0; i < proof.length; i++) {

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

File: contracts/rewards/RewardDistributor.sol
142: uint256 reward = 0;

143: for (uint256 index = 0; index < epochs.length; index++) {

156: uint256 reward = 0;

157: for (uint256 index = 0; index < epochs.length; index++) {

175: uint256 reward = 0;

176: uint256 rewardEth = 0;

180: for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {

183: for (uint256 index = 0; index < epochs.length; index++) {

222: uint256 reward = 0;

223: uint256 rewardEth = 0;

226: for (uint256 index = 0; index < epoch; index++) {

257: uint256 reward = 0;

258: for (uint256 index = 0; index < epoch; index++) {

272: uint256 reward = 0;

273: for (uint256 index = 0; index < epoch; index++) {

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

File: contracts/vote-escrow/VoteEscrowDelegation.sol
147: uint256 lower = 0;

170: uint256 votes = 0;

171: for (uint256 index = 0; index < delegated.length; index++) {

188: uint256 votes = 0;

189: for (uint256 index = 0; index < delegatednft.length; index++) {

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol

File: contracts/vote-escrow/VoteEscrowCore.sol

735: uint256 block_slope = 0; // dblock/dt

745: for (uint256 i = 0; i < 255; ++i) {

1042: uint256 _min = 0;

1044: for (uint256 i = 0; i < 128; ++i) {

1113: uint256 _min = 0;

1115: for (uint256 i = 0; i < 128; ++i) {

1133: uint256 d_block = 0;

1134: uint256 d_t = 0;

1167: for (uint256 i = 0; i < 255; ++i) {

1211: uint256 dt = 0;

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol

[G-05] Use custom errors rather than revert()/require() to save gas

Custom errors reduce the cost to deploy and call a function.

File: contracts/core/GolomTrader.sol
177: require(signaturesigner == o.signer, 'invalid signature');

211: require(

217: require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');

220: require(msg.sender == o.reservedAddress);

222: require(o.orderType == 0, 'invalid orderType');

226: require(status == 3, 'order not valid');

227: require(amountRemaining >= amount, 'order already filled');

235: require(amount == 1, 'only 1 erc721 at 1 time');

285: require(

291: require(msg.sender == o.reservedAddress);

293: require(o.orderType == 1);

295: require(status == 3);

296: require(amountRemaining >= amount);

299: require(amount == 1, 'only 1 erc721 at 1 time');

313: require(o.signer == msg.sender);

342: require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);

345: require(msg.sender == o.reservedAddress);

347: require(o.orderType == 2);

349: require(status == 3);

350: require(amountRemaining >= amount);

359: require(amount == 1, 'only 1 erc721 at 1 time');

426: revert('invalid proof');

455: require(amount == 1, 'only 1 erc721 at 1 time');

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

File: contracts/governance/GolomToken.sol
24: require(msg.sender == minter, 'GolomToken: only reward distributor can enable');

43: require(!isAirdropMinted, 'already minted');

51: require(!isGenesisRewardMinted, 'already minted');

69: require(minterEnableDate <= block.timestamp, 'GolomToken: wait for timelock');

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/governance/GolomToken.sol

File: contracts/rewards/RewardDistributor.sol
88: require(msg.sender == trader);

144: require(epochs[index] < epoch);

158: require(epochs[index] < epoch);

173: require(address(ve) != address(0), ' VE not added yet');

181: require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together');

184: require(epochs[index] < epoch, 'cant claim for future epochs');

185: require(claimed[tokenids[tindex]][epochs[index]] == 0, 'cant claim if already claimed');

220: require(address(ve) != address(0), ' VE not added yet');

292: require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');

309: require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');

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

File: contracts/vote-escrow/VoteEscrowCore.sol
360: require(_entered_state == _not_entered);

538: require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

540: require(_isApprovedOrOwner(_sender, _tokenId));

608: revert('ERC721: transfer to non ERC721Receiver implementer');

646: require(owner != address(0));

648: require(_approved != owner);

652: require(senderIsOwner || senderIsApprovedForAll);

869: require(msg.sender == voter);

874: require(msg.sender == voter);

879: require(msg.sender == voter);

884: require(msg.sender == voter);

889: require(msg.sender == voter);

894: require(attachments[_from] == 0 && !voted[_from], 'attached');

895: require(_from != _to);

896: require(_isApprovedOrOwner(msg.sender, _from));

897: require(_isApprovedOrOwner(msg.sender, _to));

927: require(_value > 0); // dev: need non-zero value

928: require(_locked.amount > 0, 'No existing lock found');

929: require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');

944: require(_value > 0); // dev: need non-zero value

945: require(unlock_time > block.timestamp, 'Can only lock until time in the future');

946: require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');

982: require(_locked.amount > 0, 'No existing lock found');

983: require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');

996: require(_locked.end > block.timestamp, 'Lock expired');

997: require(_locked.amount > 0, 'Nothing is locked');

998: require(unlock_time > _locked.end, 'Can only increase lock duration');

999: require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');

1008: require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

1011: require(block.timestamp >= _locked.end, "The lock didn't expire");

1082: require(idToOwner[_tokenId] != address(0), 'Query for nonexistent token');

1227: require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol

File: contracts/vote-escrow/VoteEscrowDelegation.sol
72: require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

73: require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');

99: require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

130: require(blockNumber < block.number, 'VEDelegation: not yet determined');

186: require(blockNumber < block.number, 'VEDelegation: not yet determined');

211: require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

239: require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

245: require(_isApprovedOrOwner(_sender, _tokenId));

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol

[G-06] Duplicted require statements/validation logic should be refactored to function modifier to save gas

File: contracts/vote-escrow/VoteEscrowCore.sol#L868-L891
function setVoter(address _voter) external {
    require(msg.sender == voter);
    voter = _voter;
}

function voting(uint256 _tokenId) external {
    require(msg.sender == voter);
    voted[_tokenId] = true;
}

function abstain(uint256 _tokenId) external {
    require(msg.sender == voter);
    voted[_tokenId] = false;
}

function attach(uint256 _tokenId) external {
    require(msg.sender == voter);
    attachments[_tokenId] = attachments[_tokenId] + 1;
}

function detach(uint256 _tokenId) external {
    require(msg.sender == voter);
    attachments[_tokenId] = attachments[_tokenId] - 1;
}

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

[G-07] x += y or x -= y costs more gas than x = x + y or x = x - y for state variables

File: contracts/vote-escrow/VoteEscrowCore.sol
499: ownerToNFTokenCount[_to] += 1;

512: ownerToNFTokenCount[_from] -= 1;

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol