feat(security-monitor): add security monitor contract #955
Conversation
GregDixonMXN
force-pushed
the
issue_#927_Security_Monitor
branch
from
0c88681
to
3dafd2d
Compare
| @@ -1,5 +1,5 @@ | |||
| # Diamond | |||
| [Git Source](https://github.com/ubiquity/ubiquity-dollar/blob/565aaa6bed7cb481fd57c9fc6a7b1052ff2aa816/src/dollar/Diamond.sol) | |||
| [Git Source](https://github.com/ubiquity/ubiquity-dollar/blob/0cae71618450aff584ed3369a18e2ba12900dc6b/src/dollar/Diamond.sol) | |||
There was a problem hiding this comment.
Why are there so many md files changes? There's a separate docs generation workflow that updates docs after each push to the development branch so there's no need update docs manually. Besides https://github.com/ubiquity/ubiquity-dollar/blob/0cae71618450aff584ed3369a18e2ba12900dc6b/src/dollar/Diamond.sol leads to unknown file so it seems that commit 0cae71618450aff584ed3369a18e2ba12900dc6b does not exist.
There was a problem hiding this comment.
im not sure maybe because i had to force push some changes after i created the PR? i didnt do this intentionally
There was a problem hiding this comment.
Overall:
- All
pause()andunpause()methods should be removed - Log based upkeep should be used
- Tests should use real contract instances, not mocks
- It's still not clear how to send telegram notifications in case of a security incident
| @@ -51,6 +50,14 @@ contract UbiquityDollarToken is ERC20Ubiquity { | |||
| address account, | |||
| uint256 amount | |||
| ) public override onlyDollarBurner whenNotPaused { | |||
| uint256 currentAllowance = allowance(account, _msgSender()); | |||
There was a problem hiding this comment.
Why was the UbiquityDollarToken contract modified? And how is this code change connected with the "security monitoring" feature?
| @@ -46,6 +46,16 @@ contract UbiquityPoolFacet is IUbiquityPool, Modifiers { | |||
| return LibUbiquityPool.collateralUsdBalance(); | |||
| } | |||
|
|
|||
| /// @inheritdoc IUbiquityPool | |||
| function pause() external onlyAdmin { | |||
There was a problem hiding this comment.
There's no need to update any of the existing contracts, libs or interfaces. We can simply disable collateral (as stated in the issue description) which will take the same effect as pausing.
| bytes calldata /* checkData */ | ||
| ) external view returns (bool upkeepNeeded) { | ||
| upkeepNeeded = | ||
| (block.timestamp - _securityStorage.lastCheckTimestamp) >= |
There was a problem hiding this comment.
Log based upkeep should be used.
| } | ||
|
|
||
| function testSecurityIncidentLiquidity() public { | ||
| mockUPool.setCollateralUsdBalance(50); // Set a low balance to trigger the incident |
There was a problem hiding this comment.
Pls use real contract instances in the tests. Mocks are prone to errors.
| assertTrue(mockUPool.paused(), "UPool should be paused"); | ||
| } | ||
|
|
||
| function testSecurityIncidentCollateralRatio() public { |
There was a problem hiding this comment.
I don't understand the purpose of this test:
- Collateral ratio is set to 80%
- Security monitor contract performs an upkeep
- Suddenly everything is paused
Why should we pause contracts in this case?
Resolves #927