Quadrata security record

An issuer could mistakenly burn a passport with valid attributes through the function `burnPassportsIssuer()`. In the following statements, if `msg.sender` does not have a position on a non-empty attribute, the variable `isEmpty` could remain true, and the function `_burnPassports()` would be triggered to burn this passport even though it contains valid attributes. Moreover, the attributes of the burned passport can still be accessed. ```solidity=246 bool isEmpty = true; // only delete attributes from issuer for (uint256 i = 0; i < governance.getEligibleAttributesLength(); i++) { bytes32 attributeType = governance.eligibleAttributesArray(i); bytes32 attrKey = keccak256(abi.encode(_account, attributeType)); uint256 position = _position[keccak256(abi.encode(attrKey, _msgSender()))]; if (position > 0) { Attribute[] storage attrs = _attributes[attrKey]; // Swap last attribute position with position of attribute to delete before calling pop() Attribute memory attrToDelete = attrs[position-1]; Attribute memory attrToSwap = attrs[attrs.length-1]; _position[keccak256(abi.encode(attrKey, attrToSwap.issuer))] = position; _position[keccak256(abi.encode(attrKey, attrToDelete.issuer))] = 0; attrs[position-1] = attrToSwap; attrs.pop(); if (attrs.length > 0) { isEmpty = false; } } } if (isEmpty){ _burnPassports(_account); } ```

```solidity bytes32 extractionHash = keccak256( abi.encode( _account, _config.attrKeys, _config.attrValues, _config.did, _config.verifiedAt, _config.issuedAt, _config.fee, block.chainid, address(this) ) ); ``` The `_setAttributesVerify()` function in the contract verifies a signature based on a specific set of input parameters. However, the `_config.tokenId` and `_config.attrTypes` variables are not included in the signature verification process. This could potentially allow an attacker to exploit the vulnerability and mint a token with an unexpected tokenId or assign `attrValues` in an unexpected order corresponding to `attrTypes`.

Passport `DID` is the ID of a user, which allows apps to determine if a user owns more than one wallet. The internal function `_validateDid()` will ensure the passport `DID` cannot be changed if trying to set an attribute with a changed `config.did` value. However, passport `DID` can be changed with an issuer signed `config` as the following: ```typescript var config = { attrValues: [didToChange], attrTypes: [ATTRIBUTE_DID], // other parameters } ```

The `attributeMetadata()` function is used to return the `issuedAt` variable of an attribute. ```solidity issuedAts[attributeIndex] = attribute.epoch; ``` However, in the `_setAttributesInternal()` function where the attribute is written, the `epoch` variable is actually the `verifiedAt` variable. As a result, the `attributeMetadata()` function may return an inaccurate `issuedAt` value. ```solidity _writeAttrToStorage( _computeAttrKey(_account, ATTRIBUTE_DID, _config.did, _issuer), _config.did, _config.verifiedAt); ```

```solidity @param _attrKey attribute key (i.e. keccak256(address, keccak256("AML"))) ``` The comment of the function `_writeAttrToStorage()` is incorrect, an example of the attribute key is `keccak256(address, keccak256("AML"), address)`. The `setAttributesIssuer()` function is named and commented to imply that it should only be called by authorized issuers. However, it can be called by anyone.

One of the goals of this fork was in protecting protocol interactions (supplying funds, borrowing, repaying) with AML (Anti-Money Laundering) score checks, as performed through the `QuadPassport.sol`-related contracts. However, it is still possible to supply funds to the pools through the `_addReserves()` functions, as exposed through `CErc20.sol` and `CEther.sol` to `CToken.sol`. While internally these funds are treated separately as part of the `totalReserves` from the usable funds and can only be used to be passed through to the `admin` address through `Ctoken._reduceReserves()` by the admin, it is unclear if it remains AML-unchecked on purpose or not.

In order to validate the proper deployment and initialization of the contracts, it is a good practice to emit events. Also, any important state transition can be logged, which is beneficial for monitoring the contract and also tracking any eventual bugs or potential hacks. Below we present a non-exhaustive list of events that could be emitted to improve the application management: 1. `ComptrollerG7._setQuadReader()` does not emit an event reflecting changes made to the state variable `reader`. 1. `UniswapAnchoredViewProxy.setUniswapAnchoredView()` does not emit an event reflecting changes made to the state variable `uav`. 1. `UniswapAnchoredViewProxy.setTargetDecimals()` does not emit an event reflecting changes made to the state variable `targetDecimals`. 1. `UniswapAnchoredViewProxy.setPriceDecimals()` does not emit an event reflecting changes made to the state variable `priceDecimals`.

In function `CToken.getHighestAMLScore()` a call is made to `getAttributesFree()` of the `QuadReader.sol` contract, where the second parameter, `_tokenId`, is provided as a hard coded `1`. However, while currently `1` is the default, there exists functionality for adding/changing eligible IDs, through `QuadGovernance.setEligibleTokenId()`. Therefore making the current setup on the `CToken.sol` side not future-proof.

Integer overflow/underflow occur when an integer hits its bit-size limit. Every integer has a set range; when that range is passed, the value loops back around. A clock is a good analogy: at 11:59, the minute hand goes to 0, not 60, because 59 is the largest possible minute. <br><br> No overflow check for the variable `value` in function `UniswapAnchoredViewProxy.scalePriceInternal(...)`. The code is reproduced below. ```javascript= function scalePriceInternal(uint248 value) internal view returns(uint248) { int256 mantissa = int256(targetDecimals) - int256(priceDecimals); bool signed = mantissa > 0; if(!signed) { mantissa *= -1; } if(signed) { value *= (10 ** uint248(mantissa)); } else { value /= (10 ** uint248(mantissa)); } return uint248(value); } ```

To prevent confusion and potentially introduce vulnerabilities, test code should not be present in production code/should be clearly separated. In this regard, contract `SimplePriceOracle.sol` constitutes an oracle test contract (as prices can be set arbitrarily by anyone manually) and should therefore be moved into a dedicated `test` or `mock` folder.