-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: create SecurityMonitorFacet and add associated mock and tests #954
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,163 @@ | ||
// SPDX-License-Identifier: Apache-2.0 | ||
pragma solidity ^0.8.19; | ||
|
||
import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; | ||
import "@openzeppelin/contracts/utils/math/SafeMath.sol"; | ||
import "../../../src/dollar/libraries/LibDiamond.sol"; | ||
import "../../../src/dollar/libraries/LibAccessControl.sol"; | ||
import "../../../lib/chainlink-brownie-contracts/contracts/src/v0.8/AutomationCompatible.sol"; | ||
import "../../../src/dollar/interfaces/IUbiquityPool.sol"; | ||
import "../../../src/dollar/interfaces/IUbiquityDollarToken.sol"; | ||
import "../../../src/dollar/interfaces/ITelegramNotifier.sol"; | ||
|
||
contract SecurityMonitorFacet is | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
ReentrancyGuard, | ||
AutomationCompatibleInterface | ||
{ | ||
using SafeMath for uint256; | ||
|
||
bytes32 public constant SECURITY_MONITOR_ROLE = | ||
keccak256("SECURITY_MONITOR_ROLE"); | ||
bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); | ||
|
||
uint256 public constant LIQUIDITY_THRESHOLD = 70; // 70% of initial liquidity | ||
uint256 public constant COLLATERAL_RATIO_THRESHOLD = 900000; // 90% (using 1e6 precision) | ||
uint256 public constant LARGE_TRANSFER_THRESHOLD = 1000000e18; // 1 million dollars | ||
|
||
struct SecurityMonitorStorage { | ||
uint256 lastCheckTimestamp; | ||
uint256 checkInterval; | ||
} | ||
|
||
bytes32 constant SECURITY_MONITOR_STORAGE_POSITION = | ||
keccak256("security.monitor.storage"); | ||
|
||
function securityMonitorStorage() | ||
internal | ||
pure | ||
returns (SecurityMonitorStorage storage s) | ||
{ | ||
bytes32 position = SECURITY_MONITOR_STORAGE_POSITION; | ||
assembly { | ||
s.slot := position | ||
} | ||
} | ||
|
||
event SecurityIncident(string message); | ||
|
||
function initialize(uint256 _checkInterval) external { | ||
LibDiamond.enforceIsContractOwner(); | ||
SecurityMonitorStorage storage s = securityMonitorStorage(); | ||
s.checkInterval = _checkInterval; | ||
s.lastCheckTimestamp = block.timestamp; | ||
} | ||
|
||
function checkUpkeep( | ||
bytes calldata /* checkData */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
) | ||
external | ||
view | ||
override | ||
returns (bool upkeepNeeded, bytes memory performData) | ||
{ | ||
SecurityMonitorStorage storage s = securityMonitorStorage(); | ||
|
||
upkeepNeeded = | ||
(block.timestamp - s.lastCheckTimestamp) >= s.checkInterval; | ||
|
||
performData = ""; // Return an empty bytes array | ||
} | ||
|
||
function performUpkeep(bytes calldata /* performData */) external override { | ||
SecurityMonitorStorage storage s = securityMonitorStorage(); | ||
|
||
if ((block.timestamp - s.lastCheckTimestamp) >= s.checkInterval) { | ||
s.lastCheckTimestamp = block.timestamp; | ||
|
||
checkSecurityConditions(); | ||
} | ||
} | ||
|
||
function checkSecurityConditions() internal { | ||
if (checkLiquidity()) { | ||
handleSecurityIncident("Liquidity below threshold"); | ||
} | ||
|
||
if (checkCollateralRatio()) { | ||
handleSecurityIncident("Collateral ratio below threshold"); | ||
} | ||
|
||
// Add more security checks as needed | ||
} | ||
|
||
function handleSecurityIncident( | ||
string memory message | ||
) internal nonReentrant { | ||
require( | ||
LibAccessControl.hasRole(SECURITY_MONITOR_ROLE, address(this)), | ||
"SecurityMonitor: not authorized" | ||
); | ||
|
||
// Pause the UbiquityDollarToken and UbiquityPoolFacet | ||
|
||
if (LibAccessControl.hasRole(PAUSER_ROLE, address(this))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check in unnecessary since we'll grant the |
||
// Instead of directly calling pause(), we'll use a more generic approach | ||
|
||
// that should work regardless of how pausing is implemented in UbiquityDollarToken | ||
|
||
(bool success, ) = address(LibDiamond.contractOwner()).call( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you try that it works as expected? Contract owner is an EOA so I don't fully understand how we can call the |
||
abi.encodeWithSignature("pauseDollarToken()") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no such method |
||
); | ||
|
||
require(success, "Failed to pause UbiquityDollarToken"); | ||
|
||
IUbiquityPool(LibDiamond.contractOwner()).toggleMintRedeemBorrow( | ||
0, | ||
0 | ||
); // Pause minting | ||
|
||
IUbiquityPool(LibDiamond.contractOwner()).toggleMintRedeemBorrow( | ||
0, | ||
1 | ||
); // Pause redeeming | ||
|
||
emit SecurityIncident(message); | ||
|
||
ITelegramNotifier(LibDiamond.contractOwner()).notify(message); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} else { | ||
emit SecurityIncident( | ||
"Failed to pause: SecurityMonitor lacks PAUSER_ROLE" | ||
); | ||
|
||
ITelegramNotifier(LibDiamond.contractOwner()).notify( | ||
"Failed to pause: SecurityMonitor lacks PAUSER_ROLE" | ||
); | ||
} | ||
} | ||
|
||
function checkLiquidity() internal view returns (bool) { | ||
uint256 poolLiquidity = IUbiquityPool(LibDiamond.contractOwner()) | ||
.collateralUsdBalance(); | ||
|
||
uint256 thresholdLiquidity = poolLiquidity.mul(LIQUIDITY_THRESHOLD).div( | ||
100 | ||
); | ||
|
||
return poolLiquidity < thresholdLiquidity; | ||
} | ||
|
||
function checkCollateralRatio() internal view returns (bool) { | ||
uint256 currentRatio = IUbiquityPool(LibDiamond.contractOwner()) | ||
.collateralRatio(); | ||
|
||
return currentRatio < COLLATERAL_RATIO_THRESHOLD; | ||
} | ||
|
||
function setCheckInterval(uint256 _newInterval) external { | ||
LibDiamond.enforceIsContractOwner(); | ||
|
||
SecurityMonitorStorage storage s = securityMonitorStorage(); | ||
|
||
s.checkInterval = _newInterval; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// SPDX-License-Identifier: MIT | ||
|
||
pragma solidity ^0.8.19; | ||
|
||
interface ITelegramNotifier { | ||
/** | ||
* @notice Sends a notification message via Telegram | ||
* @param message The message to be sent | ||
*/ | ||
|
||
function notify(string memory message) external; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// SPDX-License-Identifier: MIT | ||
|
||
pragma solidity ^0.8.19; | ||
|
||
import "../interfaces/ITelegramNotifier.sol"; | ||
|
||
contract MockTelegramNotifier is ITelegramNotifier { | ||
event NotificationSent(string message); | ||
|
||
bool public lastNotificationSent; | ||
|
||
string public lastNotificationMessage; | ||
|
||
function notify(string memory message) external override { | ||
lastNotificationSent = true; | ||
|
||
lastNotificationMessage = message; | ||
|
||
emit NotificationSent(message); | ||
} | ||
|
||
function resetNotification() external { | ||
lastNotificationSent = false; | ||
|
||
lastNotificationMessage = ""; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this package used anywhere? If not then it should be removed, chainlink contracts are already added as a github submodule here.