Skip to content

Commit

Permalink
add RBAC to LinkMon
Browse files Browse the repository at this point in the history
  • Loading branch information
Borja Aranda committed Dec 3, 2023
1 parent 1a1f9ac commit 9ffe94a
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 73 deletions.
72 changes: 45 additions & 27 deletions contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

pragma solidity 0.8.19;

import "@openzeppelin/contracts/access/AccessControlEnumerable.sol";
import {AutomationCompatibleInterface} from "../interfaces/AutomationCompatibleInterface.sol";
import {ConfirmedOwner} from "../../shared/access/ConfirmedOwner.sol";
import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol";
import {Pausable} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/security/Pausable.sol";

interface IAggregatorProxy {
function aggregator() external view returns (address);
Expand Down Expand Up @@ -33,7 +32,10 @@ interface ILinkAvailable {
/// this is a "trusless" upkeep, meaning it does not trust the caller of performUpkeep;
/// we could save a fair amount of gas and re-write this upkeep for use with Automation v2.0+,
/// which has significantly different trust assumptions
contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationCompatibleInterface {
contract LinkAvailableBalanceMonitor is AccessControlEnumerable, AutomationCompatibleInterface {
bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");

event BalanceUpdated(address indexed addr, uint256 oldBalance, uint256 newBalance);
event FundsWithdrawn(uint256 amountWithdrawn, address payee);
event UpkeepIntervalSet(uint256 oldUpkeepInterval, uint256 newUpkeepInterval);
Expand Down Expand Up @@ -74,20 +76,38 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp

/// @param linkTokenAddress the LINK token address
constructor(
address admin,
address linkTokenAddress,
uint256 minWaitPeriodSeconds,
uint16 maxPerform,
uint16 maxCheck,
uint8 upkeepInterval
) ConfirmedOwner(msg.sender) {
) {
if (linkTokenAddress == address(0)) revert InvalidLinkTokenAddress(linkTokenAddress);
_setRoleAdmin(ADMIN_ROLE, ADMIN_ROLE);
_setRoleAdmin(EXECUTOR_ROLE, ADMIN_ROLE);
_setupRole(ADMIN_ROLE, admin);
LINK_TOKEN = IERC20(linkTokenAddress);
setMinWaitPeriodSeconds(minWaitPeriodSeconds);
setMaxPerform(maxPerform);
setMaxCheck(maxCheck);
setUpkeepInterval(upkeepInterval);
}

/// @notice Grants an address an executor role
/// @param executor address to grant executor role to
function granExecutorRole(address executor) public onlyRole(ADMIN_ROLE) {
if (executor == address(0)) revert InvalidAddress(executor);
_setupRole(EXECUTOR_ROLE, executor);
}

/// @notice Revokes the executor role from an address
/// @param executor address to revoke executor role from
function revokeExecutorRole(address executor) public onlyRole(ADMIN_ROLE) {
if (executor == address(0)) revert InvalidAddress(executor);
_revokeRole(EXECUTOR_ROLE, executor);
}

/// @notice Sets the list of subscriptions to watch and their funding parameters
/// @param addresses the list of target addresses to watch (could be direct target or IAggregatorProxy)
/// @param minBalances the list of corresponding minBalance for the target address
Expand All @@ -96,7 +116,7 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp
address[] calldata addresses,
uint96[] calldata minBalances,
uint96[] calldata topUpAmounts
) external onlyOwner {
) external onlyRoleOrAdminRole(EXECUTOR_ROLE) {
if (addresses.length != minBalances.length || addresses.length != topUpAmounts.length) {
revert InvalidWatchList();
}
Expand Down Expand Up @@ -125,12 +145,12 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp
/// @dev this function has to be compatible with the event onRampSet(address, dstChainSelector) emitted by
/// the CCIP router. Important detail to know is this event is also emitted when an onRamp is decomissioned,
/// in which case it will carry the proper dstChainSelector along with the 0x0 address
function addToWatchList(address targetAddress, uint64 dstChainSelector) public onlyOwner {
function addToWatchList(address targetAddress, uint64 dstChainSelector) public onlyRoleOrAdminRole(EXECUTOR_ROLE) {
if (s_targets[targetAddress].isActive) revert DuplicateAddress(targetAddress);
address oldAddress = s_onRampAddresses[dstChainSelector];
// if targetAddress is an existing onRamp, there's a need of cleaning the previous onRamp associated to this dstChainSelector
// there's no need to remove any other address that's not an onRamp
if (dstChainSelector > 0 && abi.encodePacked(oldAddress).length > 0) {
if (dstChainSelector > 0 && bytes(abi.encodePacked(oldAddress)).length > 0) {
removeFromWatchList(oldAddress);
}
// only add the new address if it's not 0x0
Expand All @@ -151,7 +171,7 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp

/// @notice Delete an address from the watchlist and sets the target to inactive
/// @param targetAddress the address to be deleted
function removeFromWatchList(address targetAddress) public onlyOwner returns (bool) {
function removeFromWatchList(address targetAddress) public onlyRoleOrAdminRole(EXECUTOR_ROLE) returns (bool) {
s_targets[targetAddress].isActive = false;
for (uint i; i < s_watchList.length; i++) {
if (s_watchList[i] == targetAddress) {
Expand Down Expand Up @@ -201,7 +221,7 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp
return targetsToFund;
}

function topUp(address[] memory targetAddresses) public whenNotPaused {
function topUp(address[] memory targetAddresses) public {
MonitoredAddress memory target;
uint256 localBalance = LINK_TOKEN.balanceOf(address(this));
for (uint256 idx = 0; idx < targetAddresses.length; idx++) {
Expand Down Expand Up @@ -259,9 +279,7 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp
/// @notice Gets list of subscription ids that are underfunded and returns a keeper-compatible payload.
/// @return upkeepNeeded signals if upkeep is needed
/// @return performData is an abi encoded list of subscription ids that need funds
function checkUpkeep(
bytes calldata
) external view override whenNotPaused returns (bool upkeepNeeded, bytes memory performData) {
function checkUpkeep(bytes calldata) external view override returns (bool upkeepNeeded, bytes memory performData) {
address[] memory needsFunding = sampleUnderfundedAddresses();
upkeepNeeded = needsFunding.length > 0;
performData = abi.encode(needsFunding);
Expand All @@ -278,14 +296,14 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp
/// @notice Withdraws the contract balance in the LINK token.
/// @param amount the amount of the LINK to withdraw
/// @param payee the address to pay
function withdraw(uint256 amount, address payable payee) external onlyOwner {
function withdraw(uint256 amount, address payable payee) external onlyRoleOrAdminRole(EXECUTOR_ROLE) {
if (payee == address(0)) revert InvalidAddress(payee);
LINK_TOKEN.transfer(payee, amount);
emit FundsWithdrawn(amount, payee);
}

/// @notice Sets the minimum balance for the given target address
function setMinBalance(address target, uint96 minBalance) external onlyOwner {
function setMinBalance(address target, uint96 minBalance) external onlyRoleOrAdminRole(EXECUTOR_ROLE) {
if (target == address(0)) revert InvalidAddress(target);
if (minBalance == 0) revert InvalidMinBalance(minBalance);
if (!s_targets[target].isActive) revert InvalidWatchList();
Expand All @@ -295,7 +313,7 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp
}

/// @notice Sets the minimum balance for the given target address
function setTopUpAmount(address target, uint96 topUpAmount) external onlyOwner {
function setTopUpAmount(address target, uint96 topUpAmount) external onlyRoleOrAdminRole(EXECUTOR_ROLE) {
if (target == address(0)) revert InvalidAddress(target);
if (topUpAmount == 0) revert InvalidTopUpAmount(topUpAmount);
if (!s_targets[target].isActive) revert InvalidWatchList();
Expand All @@ -305,25 +323,25 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp
}

/// @notice Update s_maxPerform
function setMaxPerform(uint16 maxPerform) public onlyOwner {
function setMaxPerform(uint16 maxPerform) public onlyRoleOrAdminRole(EXECUTOR_ROLE) {
s_maxPerform = maxPerform;
emit MaxPerformSet(s_maxPerform, maxPerform);
}

/// @notice Update s_maxCheck
function setMaxCheck(uint16 maxCheck) public onlyOwner {
function setMaxCheck(uint16 maxCheck) public onlyRoleOrAdminRole(EXECUTOR_ROLE) {
s_maxCheck = maxCheck;
emit MaxCheckSet(s_maxCheck, maxCheck);
}

/// @notice Sets the minimum wait period (in seconds) for addresses between funding
function setMinWaitPeriodSeconds(uint256 minWaitPeriodSeconds) public onlyOwner {
function setMinWaitPeriodSeconds(uint256 minWaitPeriodSeconds) public onlyRoleOrAdminRole(EXECUTOR_ROLE) {
s_minWaitPeriodSeconds = minWaitPeriodSeconds;
emit MinWaitPeriodSet(s_minWaitPeriodSeconds, minWaitPeriodSeconds);
}

/// @notice Update s_upkeepInterval
function setUpkeepInterval(uint8 upkeepInterval) public onlyOwner {
function setUpkeepInterval(uint8 upkeepInterval) public onlyRoleOrAdminRole(EXECUTOR_ROLE) {
if (upkeepInterval > 255) revert InvalidUpkeepInterval(upkeepInterval);
s_upkeepInterval = upkeepInterval;
emit UpkeepIntervalSet(s_upkeepInterval, upkeepInterval);
Expand Down Expand Up @@ -362,13 +380,13 @@ contract LinkAvailableBalanceMonitor is ConfirmedOwner, Pausable, AutomationComp
return (target.isActive, target.minBalance, target.topUpAmount);
}

/// @notice Pause the contract, which prevents executing performUpkeep
function pause() external onlyOwner {
_pause();
}

/// @notice Unpause the contract
function unpause() external onlyOwner {
_unpause();
/// @dev Modifier to make a function callable only by a certain role or the
/// admin role.
modifier onlyRoleOrAdminRole(bytes32 role) {
address sender = _msgSender();
if (!hasRole(ADMIN_ROLE, sender)) {
_checkRole(role, sender);
}
_;
}
}
49 changes: 3 additions & 46 deletions contracts/test/v0.8/automation/LinkAvailableBalanceMonitor.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ const TARGET_CHECK_GAS_LIMIT = 3_500_000

const OWNABLE_ERR = 'Only callable by owner'
const INVALID_WATCHLIST_ERR = `InvalidWatchList()`
const PAUSED_ERR = 'Pausable: paused'

const zeroLINK = ethers.utils.parseEther('0')
const oneLINK = ethers.utils.parseEther('1')
Expand Down Expand Up @@ -152,6 +151,7 @@ const setup = async () => {

lt = (await ltFactory.deploy()) as LinkToken
labm = await labmFactory.deploy(
owner.address,
lt.address,
minWaitPeriodSeconds,
maxPerform,
Expand Down Expand Up @@ -260,25 +260,7 @@ describe('LinkAvailableBalanceMonitor', () => {

it('Should not allow strangers to withdraw', async () => {
const tx = labm.connect(stranger).withdraw(oneLINK, owner.address)
await expect(tx).to.be.revertedWith(OWNABLE_ERR)
})
})

describe('pause() / unpause()', () => {
it('Should allow owner to pause / unpause', async () => {
const pauseTx = await labm.connect(owner).pause()
await pauseTx.wait()
const unpauseTx = await labm.connect(owner).unpause()
await unpauseTx.wait()
})

it('Should not allow strangers to pause / unpause', async () => {
const pauseTxStranger = labm.connect(stranger).pause()
await expect(pauseTxStranger).to.be.revertedWith(OWNABLE_ERR)
const pauseTxOwner = await labm.connect(owner).pause()
await pauseTxOwner.wait()
const unpauseTxStranger = labm.connect(stranger).unpause()
await expect(unpauseTxStranger).to.be.revertedWith(OWNABLE_ERR)
await expect(tx).to.be.reverted
})
})

Expand Down Expand Up @@ -342,7 +324,7 @@ describe('LinkAvailableBalanceMonitor', () => {
const setTxStranger = labm
.connect(stranger)
.setWatchList([watchAddress1], [oneLINK], [oneLINK])
await expect(setTxStranger).to.be.revertedWith(OWNABLE_ERR)
await expect(setTxStranger).to.be.reverted
})

it('Should revert if any of the addresses are empty', async () => {
Expand Down Expand Up @@ -425,13 +407,6 @@ describe('LinkAvailableBalanceMonitor', () => {
expect(addresses).to.deep.equalInAnyOrder([])
})

it('Should revert when paused', async () => {
const tx = await labm.connect(owner).pause()
await tx.wait()
const ethCall = labm.checkUpkeep('0x')
await expect(ethCall).to.be.revertedWith(PAUSED_ERR)
})

context('with a large set of proxies', async () => {
// in this test, we cheat a little bit and point each proxy to the same aggregator,
// which helps cut down on test time
Expand Down Expand Up @@ -524,12 +499,6 @@ describe('LinkAvailableBalanceMonitor', () => {
)
})

it('Should revert when paused', async () => {
await labm.connect(owner).pause()
const performTx = labm.connect(keeperRegistry).performUpkeep(validPayload)
await expect(performTx).to.be.revertedWith(PAUSED_ERR)
})

it('Should fund the appropriate addresses', async () => {
await aggregator1.mock.linkAvailableForPayment.returns(zeroLINK)
await aggregator2.mock.linkAvailableForPayment.returns(zeroLINK)
Expand Down Expand Up @@ -622,18 +591,6 @@ describe('LinkAvailableBalanceMonitor', () => {
})
})

context('when paused', () => {
it('Should be callable by no one', async () => {
await labm.connect(owner).pause()
const users = [owner, keeperRegistry, stranger]
for (let idx = 0; idx < users.length; idx++) {
const user = users[idx]
const tx = labm.connect(user).topUp([])
await expect(tx).to.be.revertedWith(PAUSED_ERR)
}
})
})

context('when fully funded', () => {
beforeEach(async () => {
await lt.connect(owner).transfer(labm.address, tenLINK)
Expand Down

0 comments on commit 9ffe94a

Please sign in to comment.