feat: create SecurityMonitorFacet and add associated mock and tests #954
Conversation
| @@ -14,6 +14,7 @@ | |||
| "url": "https://github.com/ubiquity/ubiquity-dollar.git" | |||
| }, | |||
| "dependencies": { | |||
| "@chainlink/contracts": "^1.2.0", | |||
There was a problem hiding this comment.
Is this package used anywhere? If not then it should be removed, chainlink contracts are already added as a github submodule here.
| import "../../../src/dollar/interfaces/IUbiquityDollarToken.sol"; | ||
| import "../../../src/dollar/interfaces/ITelegramNotifier.sol"; | ||
|
|
||
| contract SecurityMonitorFacet is |
There was a problem hiding this comment.
Security monitor contract should not be a part of the diamond (i.e. a separate facet). We'd keep things simple and have a separate SecurityMonitor contract which we'd deploy manually.
| } | ||
|
|
||
| function checkUpkeep( | ||
| bytes calldata /* checkData */ |
There was a problem hiding this comment.
Isn't it better to use log based upkeeps? Then we can catch transactions that look malicious right when they occur instead of relying on a perfect timing.
|
|
||
| // Pause the UbiquityDollarToken and UbiquityPoolFacet | ||
|
|
||
| if (LibAccessControl.hasRole(PAUSER_ROLE, address(this))) { |
There was a problem hiding this comment.
This check in unnecessary since we'll grant the PAUSER role to the security contract.
|
|
||
| // that should work regardless of how pausing is implemented in UbiquityDollarToken | ||
|
|
||
| (bool success, ) = address(LibDiamond.contractOwner()).call( |
There was a problem hiding this comment.
Did you try that it works as expected? Contract owner is an EOA so I don't fully understand how we can call the pauseDollarToken() method on it.
| // that should work regardless of how pausing is implemented in UbiquityDollarToken | ||
|
|
||
| (bool success, ) = address(LibDiamond.contractOwner()).call( | ||
| abi.encodeWithSignature("pauseDollarToken()") |
There was a problem hiding this comment.
There's no such method pauseDollarToken() in the UbiquityDollarToken contract.
|
|
||
| emit SecurityIncident(message); | ||
|
|
||
| ITelegramNotifier(LibDiamond.contractOwner()).notify(message); |
There was a problem hiding this comment.
I don't understand this line. I actually thought that services like https://defender.openzeppelin.com/ has a feature of sending telegram messages that can be set up in a few clicks. Why do we need the ITelegramNotifier interface and how is it supposed to work?
Resolves #927