# Audit Report Fixes

### Critical

#### C.1
Implemented Auditors reccomendation.
#### C.2

Comment from Antons: "Better to introduce new key ENTITY_VERSION here https://github.com/Securrency/EthereumSmartContracts/blob/64b860d27f7bf76d4fb1a21b455129348b6cd881/contracts/registry-layer/composer/item-registry/commons/ItemRegistryCommonLib.sol#L382 and use 1 more key for version. That will be more intuitive for UI."

Implemented this fix.

#### C.3
Implemented Auditors reccomendation.
#### C.4
Comment form Anton: "This case is used when for values storage we use some other contract, we may not even control. What we should do - introduce new interface IEternalStorage which inherits IItemRegistryMiddlewareValues interfaces and introduces new function setValueForItemByCreator(). We don't need to write implementation for this interface."

Implemented this fix.
#### C.5
Implemented Auditors reccomendation.
#### C.6
Implemented Auditors reccomendation.
#### C.7
Implemented Auditors reccomendation.
#### C.8
Implemented Auditors reccomendation.
#### C.9
Users can make classes how they see fit, we should not remove this funcitonality because there are some use cases, but rather warn the user

#### C.10
Implemented Auditors reccomendation.
#### C.11
Awaiting Auditors recommendation

### Major Report Fixes

#### M.1:
Changed the name of the funciton internalCall() to contractCall(), as per recommendation to change the name for better context of the function in:

item-registry/packages/wallet-interface/ItemRegistryWalletInterfaceInternalCalls.sol

#### M.2:
Implemented Auditors recommendation

#### M.3:
Implemented Auditors recommendation

#### M.4:
Implemented Auditors recommendation but not an exact copy of the code. The code used:

_items.itemLocked[itemId] = itemInput.locked;
_items.itemURI[itemId] = itemInput.uri;

#### M.5:
Awaiting Auditors recommendation

Right now implemented following code:

uint itemId = _itemsByWallets[msg.sender];
require(itemId == session.itemId,"tryCloseSession: Session may be close only for correct itemId");

#### M.6:
Removed all payable function in getters for ClassesRegistry and ItemRegistry

"Specify scenarious of receiving and withdrawal ethers": Still left to specify correct scenarios

#### M.7:
Added this to all fallback functions in Composer.

require(_impl._isContract(), "Validators registry Destination address is not a contract");

### Warnings Reports Fixes

#### W.1:
Awaiting Auditors recommendation

#### W.2:
Anton's Remarks: yes but problem may be only for 1 particular asset and only if it uses bad (not audited) logic. so task to ensure logic safety including work with session is responsibility of class creator and auditors. Also currently if it opens session - it may open one more or close the last one. so its first in last out and the top session should be safe

#### W.3:
Implemented Auditors reccomendation.

#### W.4:
Implemented Auditors reccomendation.

#### W.5:
Implemented Auditors reccomendation.

require(IERC20(address(this)).balanceOf(from) >= value,
                "erc20DepositToContract: Insufficient token balance to approve");

#### W.6:
Subik: "Implemented for ERC20 but not sure if its applicable for ERC721"
#### W.7:
Awaiting Auditors recommendation
#### W.9:
Note to auditor from Subik Ji:
Do we remove checks here? I dont know if we should remove the checks for _verifyRequestNotExists and _safeGetRequestData?
#### W.10:
Anton: "The thing about sessions - give possibility to do smth in future. mostly for callbacks. and that may require to emit events, transfer funds, etc. the possible problem - if logic creator wants - he may write the code that will manipulate not only session the call was initiated for, but also some different session. that may be potential volnurability, but 1) it involves only one asset 2) it completely based on logic 3) we will have badge "verified" for logic that passed audit. audit means its safe to use and auditors suppose to check for potential vulnerabilities described"
#### W.11:
Implemented Auditors recommendation.
#### W.12:
Implemented Auditors recommendation.
#### W.13:
Implemented Auditors recommendation.

* @dev This code will not work for tokens which take a fee on transfer,
* or deflationary tokens, because the >= initialBalance + value condition will fail.

#### W.14:
Anton: "All checks should be on logic side."
#### W.15:
Anton: "yes, thats unsafe logic. auditors service must handle. if not - only one asset in danger, not protocol"
#### W.16:
Implemented Auditors recommendation.
#### W.17:
Implemented Auditors recommendation.
#### W.18:
Implemented Auditors recommendation.
#### W.19:
Implemented Auditors recommendation.
#### W.20:
Anton: "This is left for future work."
#### W.21:
Need to rename session.expirationTime to expirationBlocknumber [NOT DONE]
#### W.22:
Anton: "The easiest fix is to just add info to documentation that it's better to deploy and call 'createLogic' in one transaction.
Another solution is a method in ILogic .getInitialOwner or something."

Added:
* @dev Must deploy and call 'createLogic' in one transaction.
#### W.23:
Awaiting Auditors recommendation
#### W.24:
Problem when changed. Added
* @dev Returns true for empty propertyIds array.
#### W.25:
Note to auditor: We are not sure that this change makes sense for composer. Please elaborate.
#### W.26:
Implemented Auditors recommendation.
#### W.27:
Awaiting Auditors recommendation

#### W.28:
Awaiting Auditors recommendation

#### I.1 ... I70: