Addressing some early audit feedback & gas optimizations

README.md

@@ -1,13 +1,8 @@
 # Codex Protocol | Codex Registry Contracts
 [![Build Status](https://travis-ci.org/codex-protocol/contract.codex-registry.svg?branch=master)](https://travis-ci.org/codex-protocol/contract.codex-registry)
 
-## To do
-- Token Ejection (similar to BiddableEscrow)
-- 100% code coverage
-- Gas optimization?
-
 ## Notes
-- If you accidentally send ERC-20 tokens to the address where this is deployed, please email contact@codexprotocol.com to discuss retrieval
+- If you accidentally send ERC-20 tokens to the address where this is deployed, please email help@codexprotocol.com to discuss retrieval
 
 ## Thanks to
 - OpenZeppelin for providing tons of contract functionality in their repo at https://github.com/OpenZeppelin/zeppelin-solidity. Licensing for files taken from that repo can be found at OZ_LICENSE.

contracts/CodexRecord.sol

@@ -15,4 +15,13 @@ contract CodexRecord is CodexRecordAccess {
    * @dev Constructor function
    */
   constructor() public ERC721Token("Codex Record", "CR") { }
+
+  /**
+   * @dev Reclaim all ERC20Basic compatible tokens
+   * @param token ERC20Basic The address of the token contract
+   */
+  function reclaimToken(ERC20Basic token) external onlyOwner {
+    uint256 balance = token.balanceOf(this);
+    token.transfer(owner, balance);
+  }
 }

contracts/CodexRecordCore.sol

@@ -47,11 +47,9 @@ contract CodexRecordCore is CodexRecordFees {
   )
     public
   {
-    // For now, all new tokens will be the last entry in the array
+    // All new tokens will be the last entry in the array
     uint256 newTokenId = allTokens.length;
-
-    // Add a new token to the allTokens array
-    super._mint(_to, newTokenId);
+    internalMint(_to, newTokenId);
 
     // Add metadata to the newly created token
     //
@@ -68,4 +66,18 @@ contract CodexRecordCore is CodexRecordFees {
       emit Minted(newTokenId, _providerId, _providerMetadataId);
     }
   }
+
+  function internalMint(address _to, uint256 _tokenId) internal {
+    require(_to != address(0));
+
+    tokenOwner[_tokenId] = _to;
+    ownedTokensCount[_to] = ownedTokensCount[_to].add(1);
+
+    ownedTokensIndex[_tokenId] = ownedTokens[_to].length;
+    ownedTokens[_to].push(_tokenId);
+
+    allTokens.push(_tokenId);
+
+    emit Transfer(address(0), _to, _tokenId);
+  }
 }

contracts/CodexRecordFees.sol

@@ -4,15 +4,15 @@ import "./CodexRecordMetadata.sol";
 import "./ERC20/ERC20.sol";
 import "./ERC900/ERC900.sol";
 
-import "./library/Pausable.sol";
+import "./library/DelayedPausable.sol";
 
 
 /**
  * @title CodexRecordFees
  * @dev Storage, mutators, and modifiers for fees when using the token.
- *  This also includes the Pausable contract for the onlyOwner modifier.
+ *  This also includes the DelayedPausable contract for the onlyOwner modifier.
  */
-contract CodexRecordFees is CodexRecordMetadata, Pausable {
+contract CodexRecordFees is CodexRecordMetadata, DelayedPausable {
 
   // Implementation of the ERC20 Codex Protocol Token, used for fees in the contract
   ERC20 public codexCoin;

contracts/CodexRecordMetadata.sol

@@ -32,17 +32,6 @@ contract CodexRecordMetadata is ERC721Token {
   //  via the tokenURI method
   string public tokenURIPrefix;
 
-  /**
-   * @dev Modifier that checks to see if the token exists
-   * @param _tokenId uint256 The token ID
-   */
-  modifier tokenExists(uint256 _tokenId) {
-    require(
-      exists(_tokenId),
-      "Token doesn't exist");
-    _;
-  }
-
   /**
    * @dev Updates token metadata hashes to whatever is passed in
    * @param _tokenId uint256 The token ID
@@ -62,8 +51,8 @@ contract CodexRecordMetadata is ERC721Token {
     string _providerId, // TODO: convert to bytes32?
     string _providerMetadataId  // TODO: convert to bytes32?
   )
-    tokenExists(_tokenId)
-    public onlyOwnerOf(_tokenId)
+    public
+    onlyOwnerOf(_tokenId)
   {
     // nameHash is only overridden if it's not a blank string, since name is a
     //  required value
@@ -112,7 +101,6 @@ contract CodexRecordMetadata is ERC721Token {
   )
     public
     view
-    tokenExists(_tokenId)
     returns (bytes32 nameHash, bytes32 descriptionHash, bytes32[] fileHashes)
   {
     return (
@@ -143,7 +131,6 @@ contract CodexRecordMetadata is ERC721Token {
   )
     public
     view
-    tokenExists(_tokenId)
     returns (string)
   {
     bytes memory prefix = bytes(tokenURIPrefix);

contracts/CodexStakeContainer.sol

@@ -2,14 +2,13 @@ pragma solidity 0.4.24;
 
 import "./ERC900/ERC900BasicStakeContainer.sol";
 
-// @TODO: Add Pausable functionality to prevent staking/unstaking?
-import "./library/Pausable.sol";
+import "./library/Ownable.sol";
 
 
 /**
  * @title CodexStakeContainer
  */
-contract CodexStakeContainer is ERC900BasicStakeContainer, Pausable {
+contract CodexStakeContainer is ERC900BasicStakeContainer, Ownable {
   /**
    * @dev Constructor function
    * @param _stakingToken ERC20 The address of the token used for staking

contracts/ERC721/ERC721Basic.sol

@@ -17,9 +17,9 @@ contract ERC721Basic {
   // bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)'));
   bytes4 constant INTERFACE_ERC721 = 0x80ac58cd;
 
-  event Transfer(address indexed _from, address indexed _to, uint256 _tokenId);
-  event Approval(address indexed _owner, address indexed _approved, uint256 _tokenId);
-  event ApprovalForAll(address indexed _owner, address indexed _operator, bool _approved);
+  event Transfer(address indexed _from, address indexed _to, uint256 indexed _tokenId);
+  event Approval(address indexed _owner, address indexed _approved, uint256 indexed _tokenId);
+  event ApprovalForAll(address indexed _owner, address indexed _operator, bool indexed _approved);
 
   function balanceOf(address _owner) public view returns (uint256 _balance);
   function ownerOf(uint256 _tokenId) public view returns (address _owner);

contracts/ERC721/ERC721BasicToken.sol

@@ -16,9 +16,9 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
   using SafeMath for uint256;
   using AddressUtils for address;
 
-  // Equals to `bytes4(keccak256("onERC721Received(address,uint256,bytes)"))`
+  // Equals to `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
   // which can be also obtained as `ERC721Receiver(0).onERC721Received.selector`
-  bytes4 constant ERC721_RECEIVED = 0xf0b9e5ba;
+  bytes4 constant ERC721_RECEIVED = 0x150b7a02;
 
   // Mapping from token ID to owner
   mapping (uint256 => address) internal tokenOwner;
@@ -41,15 +41,6 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
     _;
   }
 
-  /**
-   * @dev Checks msg.sender can transfer a token, by being owner, approved, or operator
-   * @param _tokenId uint256 ID of the token to validate
-   */
-  modifier canTransfer(uint256 _tokenId) {
-    require(isApprovedOrOwner(msg.sender, _tokenId));
-    _;
-  }
-
   /**
    * @dev Checks if the smart contract includes a specific interface.
    * @param _interfaceID The interface identifier, as specified in ERC-165.
@@ -150,9 +141,9 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
   function transferFrom(
     address _from,
     address _to,
-    uint256 _tokenId)
+    uint256 _tokenId
+  )
     public
-    canTransfer(_tokenId)
   {
     internalTransferFrom(
       _from,
@@ -164,7 +155,7 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
    * @dev Safely transfers the ownership of a given token ID to another address
    * @dev If the target address is a contract, it must implement `onERC721Received`,
    *  which is called upon a safe transfer, and return the magic value
-   *  `bytes4(keccak256("onERC721Received(address,uint256,bytes)"))`; otherwise,
+   *  `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`; otherwise,
    *  the transfer is reverted.
    * @dev Requires the msg sender to be the owner, approved, or operator
    * @param _from current owner of the token
@@ -174,9 +165,9 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
   function safeTransferFrom(
     address _from,
     address _to,
-    uint256 _tokenId)
+    uint256 _tokenId
+  )
     public
-    canTransfer(_tokenId)
   {
     internalSafeTransferFrom(
       _from,
@@ -189,7 +180,7 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
    * @dev Safely transfers the ownership of a given token ID to another address
    * @dev If the target address is a contract, it must implement `onERC721Received`,
    *  which is called upon a safe transfer, and return the magic value
-   *  `bytes4(keccak256("onERC721Received(address,uint256,bytes)"))`; otherwise,
+   *  `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`; otherwise,
    *  the transfer is reverted.
    * @dev Requires the msg sender to be the owner, approved, or operator
    * @param _from current owner of the token
@@ -201,9 +192,9 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
     address _from,
     address _to,
     uint256 _tokenId,
-    bytes _data)
+    bytes _data
+  )
     public
-    canTransfer(_tokenId)
   {
     internalSafeTransferFrom(
       _from,
@@ -215,15 +206,29 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
   function internalTransferFrom(
     address _from,
     address _to,
-    uint256 _tokenId)
+    uint256 _tokenId
+  )
     internal
   {
-    require(_from != address(0));
+    address owner = ownerOf(_tokenId);
+    require(_from == owner);
     require(_to != address(0));
 
-    clearApproval(_from, _tokenId);
-    removeTokenFrom(_from, _tokenId);
-    addTokenTo(_to, _tokenId);
+    address sender = msg.sender;
+
+    require(
+      sender == owner || isApprovedForAll(owner, sender) || getApproved(_tokenId) == sender,
+      "Not authorized to transfer"
+    );
+
+    // Resetting the approved address if it's set
+    if (tokenApprovals[_tokenId] != address(0)) {
+      tokenApprovals[_tokenId] = address(0);
+    }
+
+    tokenOwner[_tokenId] = _to;
+    ownedTokensCount[_from] = ownedTokensCount[_from].sub(1);
+    ownedTokensCount[_to] = ownedTokensCount[_to].add(1);
 
     emit Transfer(_from, _to, _tokenId);
   }
@@ -232,7 +237,8 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
     address _from,
     address _to,
     uint256 _tokenId,
-    bytes _data)
+    bytes _data
+  )
     internal
   {
     internalTransferFrom(_from, _to, _tokenId);
@@ -246,66 +252,6 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
     );
   }
 
-  /**
-   * @dev Returns whether the given spender can transfer a given token ID
-   * @param _spender address of the spender to query
-   * @param _tokenId uint256 ID of the token to be transferred
-   * @return bool whether the msg.sender is approved for the given token ID,
-   *  is an operator of the owner, or is the owner of the token
-   */
-  function isApprovedOrOwner(address _spender, uint256 _tokenId) internal view returns (bool) {
-    address owner = ownerOf(_tokenId);
-    return _spender == owner || getApproved(_tokenId) == _spender || isApprovedForAll(owner, _spender);
-  }
-
-  /**
-   * @dev Internal function to mint a new token
-   * @dev Reverts if the given token ID already exists
-   * @param _to The address that will own the minted token
-   * @param _tokenId uint256 ID of the token to be minted by the msg.sender
-   */
-  function _mint(address _to, uint256 _tokenId) internal {
-    require(_to != address(0));
-    addTokenTo(_to, _tokenId);
-    emit Transfer(address(0), _to, _tokenId);
-  }
-
-  /**
-   * @dev Internal function to clear current approval of a given token ID
-   * @dev Reverts if the given address is not indeed the owner of the token
-   * @param _owner owner of the token
-   * @param _tokenId uint256 ID of the token to be transferred
-   */
-  function clearApproval(address _owner, uint256 _tokenId) internal {
-    require(ownerOf(_tokenId) == _owner);
-    if (tokenApprovals[_tokenId] != address(0)) {
-      tokenApprovals[_tokenId] = address(0);
-      emit Approval(_owner, address(0), _tokenId);
-    }
-  }
-
-  /**
-   * @dev Internal function to add a token ID to the list of a given address
-   * @param _to address representing the new owner of the given token ID
-   * @param _tokenId uint256 ID of the token to be added to the tokens list of the given address
-   */
-  function addTokenTo(address _to, uint256 _tokenId) internal {
-    require(tokenOwner[_tokenId] == address(0));
-    tokenOwner[_tokenId] = _to;
-    ownedTokensCount[_to] = ownedTokensCount[_to].add(1);
-  }
-
-  /**
-   * @dev Internal function to remove a token ID from the list of a given address
-   * @param _from address representing the previous owner of the given token ID
-   * @param _tokenId uint256 ID of the token to be removed from the tokens list of the given address
-   */
-  function removeTokenFrom(address _from, uint256 _tokenId) internal {
-    require(ownerOf(_tokenId) == _from);
-    ownedTokensCount[_from] = ownedTokensCount[_from].sub(1);
-    tokenOwner[_tokenId] = address(0);
-  }
-
   /**
    * @dev Internal function to invoke `onERC721Received` on a target address
    * @dev The call is not executed if the target address is not a contract
@@ -316,13 +262,26 @@ contract ERC721BasicToken is ERC721Basic, ERC165 {
    * @return whether the call correctly returned the expected magic value
    */
   function checkAndCallSafeTransfer(
-    address _from, address _to, uint256 _tokenId, bytes _data) internal returns (bool)
+    address _from,
+    address _to,
+    uint256 _tokenId,
+    bytes _data
+  )
+    internal
+    returns (bool)
   {
     if (!_to.isContract()) {
       return true;
     }
 
-    bytes4 retval = ERC721Receiver(_to).onERC721Received(_from, _tokenId, _data);
+    bytes4 retval = ERC721Receiver(_to)
+      .onERC721Received(
+        msg.sender,
+        _from,
+        _tokenId,
+        _data
+      );
+
     return (retval == ERC721_RECEIVED);
   }
 }