Golom contest - jayphbee's results

Judge has assessed an item in Issue #497 as Medium risk. The relevant finding follows:

Lines of code

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

Vulnerability details

Impact

User may lose funds and the lost funds will be stucked in the GolomTrader contract forever.

Proof of Concept

User may unexpectedly send ether greater than o.totalAmt * amount + p.paymentAmt

GolomTrader.sol#L217

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

when calling fillAsk, but the remaining msg.value - o.totalAmt * amount + p.paymentAmt ether didn't give back to user. And the GolomTrader contract didn't implement a method to recovery user's funds.

Recommended Mitigation Steps

Give back use's remaining funds at end of fillAsk execution

        ...

        distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);
 --->   payable(msg.sender).transfer(msg.value - o.totalAmt * amount + p.paymentAmt);
        emit OrderFilled(o.signer, msg.sender, 0, hashStruct, o.totalAmt * amount);

OR

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

QA report

Low risk issues

[L-01] ecrecover not check the recovered address is 0.

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

        address signaturesigner = ecrecover(hash, o.v, o.r, o.s);
        require(signaturesigner == o.signer, 'invalid signature');

The best practice is always check if the recovered address is 0 or not.

[L-02] Use safeTransferFrom instead of transferFrom.

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

if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
        } else {
            ERC1155(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId, amount, '');
        }

In the fillAsk function, the receiver parameter is specified by caller. There is a chance that the caller may accidently pass a contract address without implementing a method to transfer the NFT which will lead to the NFT be stucked into the contract forever.

Non critical

[N-01] type hash calculation doesn't follow the EIP712 standard.

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

    struct Order {
        ...
    }

    struct Payment {
        uint256 paymentAmt;
        address paymentAddress;
    }

Order type hash is calculated with: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L132

keccak256(
                        'order(address collection,uint256 tokenId,address signer,uint256 orderType,uint256 totalAmt,payment exchange,payment prePayment,bool isERC721,uint256 tokenAmt,uint256 refererrAmt,bytes32 root,address reservedAddress,uint256 nonce,uint256 deadline)payment(uint256 paymentAmt,address paymentAddress)'
                    ),

The 'ordershould be capitalized in accordance with theOrderstrcut declaration above. Thepayment` type hash calculation has the same issue. https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L116

keccak256('payment(uint256 paymentAmt,address paymentAddress)'),

The payment should be capitalized either.

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-encodetype

[N-02] Misleading naming convention.

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

        EIP712_DOMAIN_TYPEHASH = keccak256(
            abi.encode(
                keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'),
                keccak256(bytes('GOLOM.IO')),
                keccak256(bytes('1')),
                chainId,
                address(this)
            )
        );
    }

EIP712_DOMAIN_TYPEHASH should be named EIP712_DOMAIN_SEPERATOR as per the EIP712 standard.

EIP712_DOMAIN_TYPEHASH is keccak256 hash of 'EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'

Gas Optimizations

[G-01] Redundent nonReentrant modifier.

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

function cancelOrder(Order calldata o) public nonReentrant 
function incrementNonce() external nonReentrant 

There's no need to add the nonReentrant modifier, because there's no untrusted external call in the two functions. This will save one sload and sstore.

[G-02] Cache the storage variable

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

        if (filled[hashStruct] >= o.tokenAmt) {
            // handles erc1155
            return (2, hashStruct, 0);
        }
        return (3, hashStruct, o.tokenAmt - filled[hashStruct]);

the filled[hashStruct] is read from storage twice. cache the value will save one sload gas.

[G-03] Cache the staticcall return value.

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

            uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
                rewardToken.totalSupply();
            uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();

rewardToken.totalSupply() is called 3 times. Cache the value will save two staticall gas usage.

[G-04] RewardDistributor.sol::addFee can be declared as payable to receive fee from GolomTrader contract.

fillAsk is a hot path. It will collect fees and send to RewardDistributor contract by payEther https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L242

// pay fees of 50 basis points to the distributor
payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));

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

distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);

We can eliminate the payEther call in L242 instead sending the fee in addFee by declare it as payable and remove the fee parameter.

distributor.addFee{value: ((o.totalAmt * 50) / 10000) * amount}([o.signer, o.exchange.paymentAddress]);

By doing this, the fallback and receive function can also be deleted in RewardDistributor contract, because they are no longer used to receive ether from GolomTrader contract.

The issue also applies to _settleBalances https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L375-L403

[G-05] rewardToken and weth can be decalared as immutable.

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

    ERC20 public rewardToken;
    ERC20 public weth;

rewardToken and weth are only initialized in constructor https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L74-L84

    constructor(
        address _weth,
        address _trader,
        address _token,
        address _governance
    ) {
        weth = ERC20(_weth);
        trader = _trader;
        rewardToken = ERC20(_token);
        _transferOwnership(_governance); // set the new owner
        startTime = 1659211200;

Declaring them as immutable will save gas.

[G-06] Use custome error can save gas.

This codebase is based on solidity 0.8.11, using custome error can both save deployment gas and runtime gas.

https://blog.soliditylang.org/2021/04/21/custom-errors/