Omniscia Nevermined Audit
DIDFactory Manual Review Findings
DIDFactory Manual Review Findings
DID-01M: DID Race Condition
| Type | Severity | Location |
|---|---|---|
| Logical Fault | Major | DIDFactory.sol:L171-L182 |
Description:
The registerAttribute contains no access control as to who is able to claim a DID as the registerDID function it invokes will either update a DID owned by the caller or render the caller the owner of the DID. This allows one to inspect the mempool of an Ethereum node for a DID claim and provide a higher gas fee thereby racing it.
Example:
200function registerDID(201 bytes32 _did,202 bytes32 _checksum,203 address[] memory _providers,204 string memory _url,205 bytes32 _activityId,206 string memory _attributes207)208public209virtual210onlyValidAttributes(_attributes)211returns (uint size)212{213 require(214 didRegisterList.didRegisters[_did].owner == address(0x0) ||215 didRegisterList.didRegisters[_did].owner == msg.sender,216 'Only DID Owners'217 );218
219 uint updatedSize = didRegisterList.update(_did, _checksum, _url);220
221 // push providers to storage222 for (uint256 i = 0; i < _providers.length; i++) {223 didRegisterList.addProvider(224 _did,225 _providers[i]226 );227 }228
229 emit DIDAttributeRegistered(230 _did,231 didRegisterList.didRegisters[_did].owner,232 _checksum,233 _url,234 msg.sender,235 block.number236 );237 238 _wasGeneratedBy(239 _did, _did, msg.sender, _activityId, _attributes);240 241 return updatedSize;242}Recommendation:
We strongly recommend ownership of the DIDs to be claimed in another, privileged fashion as the current setup is susceptible to race conditions.
Alleviation:
The registration of a DID was instead revamped to calculate a hash for the respective DID via concatenating the seed with the msg.sender thereby preventing one member to race the DID of another.
DID-02M: Prevention of DID Registration
| Type | Severity | Location |
|---|---|---|
| Logical Fault | Medium | DIDFactory.sol:L238-L239 |
Description:
The _wasGeneratedBy provenance creation uses the _did as the ID of the provenance which, if consumed by another DID as part of another provenance ID, would render registration of the DID impossible.
Example:
200function registerDID(201 bytes32 _did,202 bytes32 _checksum,203 address[] memory _providers,204 string memory _url,205 bytes32 _activityId,206 string memory _attributes207)208public209virtual210onlyValidAttributes(_attributes)211returns (uint size)212{213 require(214 didRegisterList.didRegisters[_did].owner == address(0x0) ||215 didRegisterList.didRegisters[_did].owner == msg.sender,216 'Only DID Owners'217 );218
219 uint updatedSize = didRegisterList.update(_did, _checksum, _url);220
221 // push providers to storage222 for (uint256 i = 0; i < _providers.length; i++) {223 didRegisterList.addProvider(224 _did,225 _providers[i]226 );227 }228
229 emit DIDAttributeRegistered(230 _did,231 didRegisterList.didRegisters[_did].owner,232 _checksum,233 _url,234 msg.sender,235 block.number236 );237 238 _wasGeneratedBy(239 _did, _did, msg.sender, _activityId, _attributes);240 241 return updatedSize;242}Recommendation:
We advise this provenance to also accept an input as to what provenance ID it should utilize to prevent misuse as well as race-conditions from occuring.
Alleviation:
The likelihood of a hash collision between the ID generated using seed and sender concatenation and the ID consumed by another provenance proof is negligible, thus this exhibit can now be considered null.
DID-03M: Pull-Over-Push Pattern
| Type | Severity | Location |
|---|---|---|
| Input Sanitization | Minor | DIDFactory.sol:L156-L158 |
Description:
The setManager function overwrites the previously set manager with the new one without validating it.
Example:
156function setManager(address _addr) external onlyOwner {157 manager = _addr;158}Recommendation:
We advise the pull-over-push pattern to be applied whereby a new manager is first proposed and consequently needs to accept their position via another dedicated function ensuring that they are able to actuate transactions on the blockchain.
Alleviation:
Comments were added surrounding the function indicating that the transfer should occur to the TransferCondition contract address. As a result, we believe that explicit knowledge of ownership is not really warranted here as the condition implementation is built with this assumption.