Golom contest - rotcivegaf's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L370-L403

Vulnerability details

In the _settleBalances function the contract take o.totalAmt * amount and send ether to different parties(distributor, o.exchange.paymentAddress, o.prePayment.paymentAddress, referrer, maker(msg.sender), p.paymentAddress, protocolfee)

The calculate of maker(msg.sender) fee (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) * amount - p.paymentAmt and (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt, depends the path taken by the if, it's wrong. The protocolfee have the amount multiplier when it's defined uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount; This makes the amount of eth taken less than the amount sent to the parties, being locked in the contract

Proof of Concept

This equation works when de amount it's 1 but if it's greater than 1(ERC1155) the maker(msg.sender) fee decrease exponential in base of amount

Tools Used

Review

Recommended Mitigation Steps

Move the protocolfee out of amount multiplaier

    function _settleBalances(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p
    ) internal {
        uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;
        WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);
        WETH.withdraw(o.totalAmt * amount);
        payEther(protocolfee, address(distributor));
        payEther(o.exchange.paymentAmt * amount, o.exchange.paymentAddress);
        payEther(o.prePayment.paymentAmt * amount, o.prePayment.paymentAddress);
        if (o.refererrAmt > 0 && referrer != address(0)) {
            payEther(o.refererrAmt * amount, referrer);
            payEther(
-               (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
+               (o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
                    amount -
+                   protocolfee -
                    p.paymentAmt,
                msg.sender
            );
        } else {
            payEther(
-               (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
+               (o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - protocolfee - p.paymentAmt,
                msg.sender
            );
        }
        payEther(p.paymentAmt, p.paymentAddress);
        distributor.addFee([msg.sender, o.exchange.paymentAddress], protocolfee);
    }

Lines of code

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

Vulnerability details

Impact

Use call instead of transfer to send ether. And return value must be checked if sending ether is successful or not. Sending ether with the transfer is no longer recommended.

This transaction will fail inevitably when:

1. The _to smart contract does not implement a payable function.
2. _to smart contract does implement a payable fallback which uses more than 2300 gas unit.
3. The _to 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.

Proof of Concept

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

Recommended Mitigation Steps

Use .call pattern:

function payEther(uint256 payAmt, address payAddress) internal {
    if (payAmt > 0) {
        // if royalty has to be paid
        (bool result, ) = payAddress.call{value: payAmt}(""); // royalty transfer to royaltyaddress
        require(result, "Failed to send Ether");
    }
}

Lines of code

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

Vulnerability details

Impact

The fillAsk, fillBid and fillCriteriaBid functions use transferFrom to transfer ERC721 token, the receiver could be an unprepared contract

Proof of Concept

Should a ERC-721 compatible token be transferred to an unprepared contract, it would end up being locked up there. Moreover, if a contract explicitly wanted to reject ERC-721 safeTransfers.

Tools Used

Manual Review

Recommended Mitigation Steps

L236:
- ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
+ ERC721(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId);
L301:
- nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);
+ nftcontract.safeTransferFrom(msg.sender, o.signer, o.tokenId);
L361:
- nftcontract.transferFrom(msg.sender, o.signer, tokenId);
+ nftcontract.safeTransferFrom(msg.sender, o.signer, tokenId);

Also change the interface ERC721:

L9:
- function transferFrom(
+ function safeTransferFrom(

Lines of code

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

Vulnerability details

Impact

Broke the EIP 721, possible lost of NFTs if sent them to contracts that cannot handle them

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, also, the receipt contract may not be prepared to handle the NFT

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

Tools Used

Participate in the velodrome contest, the same error of: https://github.com/code-423n4/2022-05-velodrome-findings/issues/185

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

Acording the EIP 721 in the interface ERC721, function safeTransferFrom: "When transfer is complete, this function checks if _to is a smart contract (code size > 0). If so, it calls onERC721Received on _to and throws if the return value is not bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))."

function safeTransferFrom(
    address _from,
    address _to,
    uint256 _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.

Setting the severity to high as the impact is merge() and withdraw() permanent unavailability for any approved sender, who isn't the owner of the involved NFT.

Proof of Concept

_removeTokenFrom() requires _from to be the NFT owner as it removes _tokenId from the _from account:

    /// @dev Remove a NFT from a given address
    ///      Throws if `_from` is not the current owner.
    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:

    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()'s assert(idToOwner[_tokenId] == _from) check.

Now burn() is used by merge():

    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():

    /// @notice Withdraw all tokens for `_tokenId`
    /// @dev Only possible if the lock has expired
    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);
    }

Tools Used

Participate in the velodrome contest, the same error of: https://github.com/code-423n4/2022-05-velodrome-findings/issues/66

Recommended Mitigation Steps

Consider changing _removeTokenFrom() argument to be the owner:

    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);
+       _removeTokenFrom(owner, _tokenId);        
        emit Transfer(owner, address(0), _tokenId);
    }

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L227-L256 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L1226-L1236

Vulnerability details

Impact

The delegation it's not removed in _burn() function, used in merge and withdraw functions of VoteEscrowCore

Proof of Concept

In _transferFrom() calls removeDelegation() in VoteEscrow.sol to remove delegation

VoteEscrow.sol#L233-L256:

function _transferFrom(
    address _from,
    address _to,
    uint256 _tokenId,
    address _sender
) internal override {
    require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

    // remove the delegation
    this.removeDelegation(_tokenId);

    // Check requirements
    require(_isApprovedOrOwner(_sender, _tokenId));
    // Clear approval. Throws if `_from` is not the current owner
    _clearApproval(_from, _tokenId);
    // Remove NFT. Throws if `_tokenId` is not a valid NFT
    _removeTokenFrom(_from, _tokenId);
    // Add NFT
    _addTokenTo(_to, _tokenId);
    // Set the block of ownership transfer (for Flash NFT protection)
    ownership_change[_tokenId] = block.number;
    // Log the transfer
    emit Transfer(_from, _to, _tokenId);
}

But _burn() in VoteEscrowCore.sol does not to remove delegation

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);
}

Tools Used

Review

Recommended Mitigation Steps

Call this.removeDelegation(_tokenId) in _burn() And mark _burn() of VoteEscrowCore contract as virtual

function _burn(uint256 _tokenId) internal virtual {
    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);
}

function _burn(uint256 _tokenId) internal override {
    // remove the delegation
    this.removeDelegation(_tokenId);

    super._burn(_tokenId);
}

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L203-L271

Vulnerability details

Impact

In the fillAsk payable function, the sender could send more value and the function check value is greater or equal of o.totalAmt * amount + p.paymentAmt and if the value is greater this amount will stuck in the contract

Proof of Concept

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

Tools Used

Review

Recommended Mitigation Steps

The msg.value should be equal than o.totalAmt * amount + p.paymentAmt in L217

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

QA report

Non-critical

[N-01] The payment should be start with capital letter because it's a struct data type

Change the GolomTrader.sol#L132 from: '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)' To: '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)'

And in GolomTrader.sol#L116 from: keccak256('payment(uint256 paymentAmt,address paymentAddress)'), To: keccak256('Payment(uint256 paymentAmt,address paymentAddress)'),

Low Risk

[L-01] The fallback and receive, both payable are redundant

In Reward Distributor contract remove this function, this contract should not receive eth

[L-02] The startTime it's old

The startTime variable of RewardDistributor.sol contract it's set in the constructor with an old date(Sat Jul 30 2022 20:00:00 GMT+0000), this could be broke the addFee function Also the startTimecould be immutable for save gas

I recommend set the startTime as a parameter of the constructor and mark this variable as immutable

L46: uint256 public immutable startTime;
L74-L85:
    constructor(
        address _weth,
        address _trader,
        address _token,
        address _governance,
        uint256 _startTime
    ) {
        weth = ERC20(_weth);
        trader = _trader;
        rewardToken = ERC20(_token);
        _transferOwnership(_governance); // set the new owner
        startTime = _startTime;
    }