Golom contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

Function Basket.withdrawETH() uses transfer function to send ETH to address _to.

This is unsafe as transfer has hard coded gas budget (2300 gas) and can fail when the user is a smart contract.

Proof-of-concept

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

Tools Used

Manual code review

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

Lines of code

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

Vulnerability details

Impact

In the function fillAsk(), fillBid() and fillCriteriaBid(), ERC721 token is sent and the transferFrom keyword is used instead of safeTransferFrom. If receiver is a contract and is not aware of incoming ERC721 tokens, the sent tokens could be locked forever

Proof of Concept

Please refer to this report from another contest https://code4rena.com/reports/2022-05-cally#m-09-use-safetransferfrom-instead-of-transferfrom-for-erc721-transfers

Recommended Mitigation Steps

Consider changing transferFrom to safeTransferFrom.

Lines of code

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

Vulnerability details

Impact

Broke the EIP 721, possible lost of NFTs

Proof of Concept

If somebody use safeTransferFrom to send an NFT to a contract, and this contract don't revert the onERC721Received call and instead of that, reject the NFT by returning a different value of bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")), the NFT may be lost in this contract

Acording the EIP 721 in interface ERC721TokenReceiver, function onERC721Received: "Return of other than the magic value MUST result in the transaction being reverted."

Recommended Mitigation Steps

In the safeTransferFrom function check the return of the IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) call is equal to bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")) to safisfy the EIP-721

function safeTransferFrom(
    address _from,
    address _to,
    uint _tokenId,
    bytes memory _data
) public {
    _transferFrom(_from, _to, _tokenId, msg.sender);

    if (_isContract(_to)) {
        // Throws if transfer destination is a contract which does not implement 'onERC721Received'
        try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4 retval) {
            require(retval == IERC721Receiver.onERC721Received.selector, 'ERC721: transfer to non ERC721Receiver implementer');
        } catch (bytes memory reason) {
            if (reason.length == 0) {
                revert('ERC721: transfer to non ERC721Receiver implementer');
            } else {
                assembly {
                    revert(add(32, reason), mload(reason))
                }
            }
        }
    }
}

Lines of code

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

Vulnerability details

Impact

Users who are approved, but do not own a particular NFT, are supposed to be eligible to call merge and withdraw from the NFT.

Currently _burn(), used by merge() and withdraw() to remove the NFT from the system, will revert unless the sender is the owner of NFT as the function tries to update the accounting for the sender, not the owner.

Proof of Concept

_removeTokenFrom() requires _from to be the NFT owner as it removes _tokenId from the _from account: https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L506

function _removeTokenFrom(address _from, uint256 _tokenId) internal {
        // Throws if `_from` is not the current owner
        assert(idToOwner[_tokenId] == _from);
        // Change the owner
        idToOwner[_tokenId] = address(0);
        // Update owner token index tracking
        _removeTokenFromOwnerList(_from, _tokenId);
        // Change count tracking
        ownerToNFTokenCount[_from] -= 1;
}

_burn() allows _tokenId to approved or owner, but calls _removeTokenFrom() with msg.sender as _from: https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1234

function _burn(uint256 _tokenId) internal {
        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

        address owner = ownerOf(_tokenId);

        // Clear approval
        approve(address(0), _tokenId);
        // Remove token
        _removeTokenFrom(msg.sender, _tokenId);
        emit Transfer(owner, address(0), _tokenId);
}

This way if _burn() is called by an approved account who isn't an owner, it will revert on _removeTokenFrom() assert(idToOwner[_tokenId] == _from) check.

Now _burn() is used by merge(): https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L906

function merge(uint256 _from, uint256 _to) external {
        require(attachments[_from] == 0 && !voted[_from], 'attached');
        require(_from != _to);
        require(_isApprovedOrOwner(msg.sender, _from));
        require(_isApprovedOrOwner(msg.sender, _to));

        LockedBalance memory _locked0 = locked[_from];
        LockedBalance memory _locked1 = locked[_to];
        uint256 value0 = uint256(int256(_locked0.amount));
        uint256 end = _locked0.end >= _locked1.end ? _locked0.end : _locked1.end;

        locked[_from] = LockedBalance(0, 0);
        _checkpoint(_from, _locked0, LockedBalance(0, 0));
        _burn(_from);
        _deposit_for(_to, value0, end, _locked1, DepositType.MERGE_TYPE);
}

And withdraw(): https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1026

function withdraw(uint256 _tokenId) external nonreentrant {
        assert(_isApprovedOrOwner(msg.sender, _tokenId));
        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

        LockedBalance memory _locked = locked[_tokenId];
        require(block.timestamp >= _locked.end, "The lock didn't expire");
        uint256 value = uint256(int256(_locked.amount));

        locked[_tokenId] = LockedBalance(0, 0);
        uint256 supply_before = supply;
        supply = supply_before - value;

        // old_locked can have either expired <= timestamp or zero end
        // _locked has only 0 end
        // Both can have >= 0 amount
        _checkpoint(_tokenId, _locked, LockedBalance(0, 0));

        assert(IERC20(token).transfer(msg.sender, value));

        // Burn the NFT
        _burn(_tokenId);

        emit Withdraw(msg.sender, _tokenId, value, block.timestamp);
        emit Supply(supply_before, supply_before - value);
}

Recommended Mitigation Steps

Consider changing _removeTokenFrom() argument to be the owner

_removeTokenFrom(owner, _tokenId);

Lines of code

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

Vulnerability details

Impact

In GolomTrader contract, it is possible for a user filling a signed order of ordertype 0 to accidentally overpay in fillAsk()

Any excess funds paid for in excess of total value needed will be locked in the contract forever since there is no way to recover in the contract

The value of one NFT and other payments value are fixed at the time order is signed and extra payment is set by taker. Hence there is no need to allow to overpay since there will be no benefit.

Proof of Concept

fillAsk() allows msg.value > total value needed

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

Recommended Mitigation Steps

Consider modifying the check such that the msg.value is exactly equal to the total value needed. e.g.

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

Summary

1. Unnecessary fallback and receive function can lock ETH forever

In GolomTrader, there is no function to recover ETH accidentally sent to the contract. And there is no need for fallback() and receive() functions. Consider remove these 2 functions in GolomTrader

Affected Codes

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

2. Implementation is not align with documentation

Comments in GolomTrader.payEther() is not matched with the function since payEther() is used for all the payments in contract, not only royalty.

Affected Codes

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

3. Division before multiplication can lead to precision errors

Affected Codes

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

4. Typos

• refererrAmt => referrerAmt. Line 57

Summary

1. Caching returned values from function calls can save gas in addFee()

External call to other contract is gas consuming and should avoid as much as possible.

For example, in RewardDistributor.addFee() function, totalSupply() is called 3 times and balanceOf() is called 2 times. We should call only 1 time for each and cache the returned values.

Affected Codes

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

2. Total ETH value is calculated multiple times and should be cached

Total value of ETH needed to fill an order in fillAsk() is calculated repeatedly. We can cache this value to save gas.

Affected Codes

• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L212
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L253
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L263

3. Should cache returned values from ve to save gas

In multiStakerClaim() function, balanceOfAtNFT() and totalSupplyAt() is called repeatedly and should be cached to save gas.

Affected Codes

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