Golom contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

The implementation of onERC721Received fails to check the return value being equal to "IERC721Receiver.onERC721Received.selector". If this response is not obtained then transaction should revert but the current implementation does not throw error in that case. This could lead to loss of token

Proof of Concept

1. Observe the safeTransferFrom function

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) {} catch (
                bytes memory reason
            ) {
                if (reason.length == 0) {
                    revert('ERC721: transfer to non ERC721Receiver implementer');
                } else {
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        }
    }

2. Our point of interest is where it is calling onERC721Received on target contract

try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4) {}

3. As we can see it is not checking whether the return value on calling onERC721Received is "IERC721Receiver.onERC721Received.selector" which means even if the target contract returns any other magic selector value, this contract will not fail

Recommended Mitigation Steps

Add below line to implementation of onERC721Received

try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4 retVal) 
{
require(retval == IERC721Receiver.onERC721Received.selector, 'ERC721: non ERC721Receiver');
} catch
.... 

Lines of code

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

Vulnerability details

Impact

The _burn function is trying to remove tokens from msg.sender instead of owner. This means the call will fail everytime if called from an approved user. All function which calls _burn which is withdraw and merge will fail

Proof of Concept

1. merge and withdraw functions both make use of _burn function

2. Assume _burn is called by one of approved user

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

3. Since msg.sender is an approved user so below require passes

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

4. After few lines, it finally tries to remove token by calling _removeTokenFrom as shown below

_removeTokenFrom(msg.sender, _tokenId);

5. Notice that this function is asking to remove _tokenId from msg.sender. This is incorrect as msg.sender might not be the owner of this _tokenId and just an approver.

6. This means the call will failas below statement fails

function _removeTokenFrom(address _from, uint256 _tokenId) internal {
        // Throws if `_from` is not the current owner
        assert(idToOwner[_tokenId] == _from);
        ....
    }

Recommended Mitigation Steps

Kindly change L1234 so that removal is done from owner and not msg.sender

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

Function never returns

Contract: GolomTrader.sol

Issue: The validateOrder function will not return 0 value and will instead fail if signature is not valid. This is happening due to below condition:

require(signaturesigner == o.signer, 'invalid signature');

Recommendation: Kindly remove the below line:

require(signaturesigner == o.signer, 'invalid signature');

Increase nonce instead of marking order as filled

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

Issue: The order is cancelled by incrementing the filled cap of an order. This means this order will look like it was filled fully rather than cancelled

Recommendation: Instead of "filled[hashStruct] = o.tokenAmt + 1;" use ++nonces[o.signer]

Use require instead of assert

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

Issue: Assert should only be used to check internal errors and should never fail unless there is a bug in the contract. The current contract implementation uses assert at multiple places VoteEscrowCore.sol#L493, VoteEscrowCore.sol#L506, VoteEscrowCore.sol#L519 etc

Recommendation: Use require instead of assert