Skip to content

Commit

Permalink
fix: Role Admin should be handling provers, as before
Browse files Browse the repository at this point in the history
  • Loading branch information
ChiTimesChi committed Dec 4, 2024
1 parent b2bac2d commit 2f300e6
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 25 deletions.
4 changes: 2 additions & 2 deletions packages/contracts-rfq/contracts/AdminV2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ contract AdminV2 is AccessControlEnumerable, IAdminV2, IAdminV2Errors {
}

/// @inheritdoc IAdminV2
function addProver(address prover) external onlyRole(GOVERNOR_ROLE) {
function addProver(address prover) external onlyRole(DEFAULT_ADMIN_ROLE) {
if (getActiveProverID(prover) != 0) revert ProverAlreadyActive();
ProverInfo storage $ = _proverInfos[prover];
// Add the prover to the list of all provers and record its id (its position + 1),
Expand All @@ -109,7 +109,7 @@ contract AdminV2 is AccessControlEnumerable, IAdminV2, IAdminV2Errors {
}

Check notice

Code scanning / Slither

Block timestamp Low

AdminV2.addProver(address) uses timestamp for comparisons
Dangerous comparisons:
- getActiveProverID(prover) != 0

/// @inheritdoc IAdminV2
function removeProver(address prover) external onlyRole(GOVERNOR_ROLE) {
function removeProver(address prover) external onlyRole(DEFAULT_ADMIN_ROLE) {
if (getActiveProverID(prover) == 0) revert ProverNotActive();
// We never remove provers from the list of all provers to preserve their IDs,
// so we just need to reset the activeFrom timestamp.
Expand Down
4 changes: 2 additions & 2 deletions packages/contracts-rfq/contracts/interfaces/IAdminV2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ interface IAdminV2 {
event ProverRemoved(address prover);
event ProverTimeoutApplied(address prover, uint256 inactiveUntilTimestamp);

/// @notice Allows the governor to add a new prover to the contract.
/// @notice Allows the role admin to add a new prover to the contract.
function addProver(address prover) external;

/// @notice Allows the governor to remove a prover from the contract.
/// @notice Allows the role admin to remove a prover from the contract.
function removeProver(address prover) external;

/// @notice Allows the governor to set the cancel delay. The cancel delay is the time period after the transaction
Expand Down
34 changes: 17 additions & 17 deletions packages/contracts-rfq/test/FastBridgeV2.Management.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ contract FastBridgeV2ManagementTest is FastBridgeV2Test {
uint256 proverAtime = block.timestamp;
vm.expectEmit(address(fastBridge));
emit ProverAdded(proverA);
addProver(governor, proverA);
addProver(admin, proverA);
assertEq(fastBridge.getActiveProverID(proverA), 1);
assertEq(fastBridge.getActiveProverID(proverB), 0);
address[] memory provers = fastBridge.getProvers();
Expand All @@ -136,7 +136,7 @@ contract FastBridgeV2ManagementTest is FastBridgeV2Test {
uint256 proverBtime = block.timestamp;
vm.expectEmit(address(fastBridge));
emit ProverAdded(proverB);
addProver(governor, proverB);
addProver(admin, proverB);
assertEq(fastBridge.getActiveProverID(proverA), 1);
assertEq(fastBridge.getActiveProverID(proverB), 2);
address[] memory provers = fastBridge.getProvers();
Expand All @@ -154,7 +154,7 @@ contract FastBridgeV2ManagementTest is FastBridgeV2Test {
uint256 proverBtime = block.timestamp;
vm.expectEmit(address(fastBridge));
emit ProverAdded(proverB);
addProver(governor, proverB);
addProver(admin, proverB);
assertEq(fastBridge.getActiveProverID(proverA), 0);
assertEq(fastBridge.getActiveProverID(proverB), 2);
address[] memory provers = fastBridge.getProvers();
Expand All @@ -167,7 +167,7 @@ contract FastBridgeV2ManagementTest is FastBridgeV2Test {
uint256 proverAtime = block.timestamp;
vm.expectEmit(address(fastBridge));
emit ProverAdded(proverA);
addProver(governor, proverA);
addProver(admin, proverA);
assertEq(fastBridge.getActiveProverID(proverA), 1);
assertEq(fastBridge.getActiveProverID(proverB), 2);
provers = fastBridge.getProvers();
Expand All @@ -178,24 +178,24 @@ contract FastBridgeV2ManagementTest is FastBridgeV2Test {
checkProverInfo(proverB, 2, proverBtime);
}

function test_addProver_revertNotGovernor(address caller) public {
vm.assume(caller != governor);
expectUnauthorized(caller, fastBridge.GOVERNOR_ROLE());
function test_addProver_revertNotAdmin(address caller) public {
vm.assume(caller != admin);
expectUnauthorized(caller, fastBridge.DEFAULT_ADMIN_ROLE());
addProver(caller, proverA);
}

function test_addProver_revertAlreadyActive() public {
test_addProver();
vm.expectRevert(ProverAlreadyActive.selector);
addProver(governor, proverA);
addProver(admin, proverA);
}

function test_addProver_revertTooManyProvers() public {
for (uint256 i = 0; i < type(uint16).max; i++) {
addProver(governor, address(uint160(i)));
addProver(admin, address(uint160(i)));
}
vm.expectRevert(ProverCapacityExceeded.selector);
addProver(governor, proverA);
addProver(admin, proverA);
}

// ═══════════════════════════════════════════════ REMOVE PROVER ═══════════════════════════════════════════════════
Expand All @@ -205,7 +205,7 @@ contract FastBridgeV2ManagementTest is FastBridgeV2Test {
uint256 proverBtime = block.timestamp;
vm.expectEmit(address(fastBridge));
emit ProverRemoved(proverA);
removeProver(governor, proverA);
removeProver(admin, proverA);
assertEq(fastBridge.getActiveProverID(proverA), 0);
assertEq(fastBridge.getActiveProverID(proverB), 2);
address[] memory provers = fastBridge.getProvers();
Expand All @@ -219,7 +219,7 @@ contract FastBridgeV2ManagementTest is FastBridgeV2Test {
test_removeProver();
vm.expectEmit(address(fastBridge));
emit ProverRemoved(proverB);
removeProver(governor, proverB);
removeProver(admin, proverB);
assertEq(fastBridge.getActiveProverID(proverA), 0);
assertEq(fastBridge.getActiveProverID(proverB), 0);
address[] memory provers = fastBridge.getProvers();
Expand All @@ -228,21 +228,21 @@ contract FastBridgeV2ManagementTest is FastBridgeV2Test {
checkProverInfo(proverB, 2, 0);
}

function test_removeProver_revertNotGovernor(address caller) public {
vm.assume(caller != governor);
expectUnauthorized(caller, fastBridge.GOVERNOR_ROLE());
function test_removeProver_revertNotAdmin(address caller) public {
vm.assume(caller != admin);
expectUnauthorized(caller, fastBridge.DEFAULT_ADMIN_ROLE());
removeProver(caller, proverA);
}

function test_removeProver_revertNeverBeenActive() public {
vm.expectRevert(ProverNotActive.selector);
removeProver(governor, proverA);
removeProver(admin, proverA);
}

function test_removeProver_revertNotActive() public {
test_removeProver();
vm.expectRevert(ProverNotActive.selector);
removeProver(governor, proverA);
removeProver(admin, proverA);
}

// ═════════════════════════════════════════════ SET CANCEL DELAY ══════════════════════════════════════════════════
Expand Down
5 changes: 3 additions & 2 deletions packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,13 @@ abstract contract FastBridgeV2SrcBaseTest is FastBridgeV2Test {
}

function configureFastBridge() public virtual override {
fastBridge.addProver(relayerA);
fastBridge.addProver(relayerB);

fastBridge.grantRole(fastBridge.GUARD_ROLE(), guard);
fastBridge.grantRole(fastBridge.CANCELER_ROLE(), canceler);

fastBridge.grantRole(fastBridge.GOVERNOR_ROLE(), address(this));
fastBridge.addProver(relayerA);
fastBridge.addProver(relayerB);
fastBridge.setCancelDelay(PERMISSIONLESS_CANCEL_DELAY);
fastBridge.setProverTimeout(PROVER_TIMEOUT);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {IFastBridge, MulticallTargetIntegrationTest} from "./MulticallTarget.t.s
contract FastBridgeV2MulticallTargetTest is MulticallTargetIntegrationTest {
function deployAndConfigureFastBridge() public override returns (address) {
FastBridgeV2 fb = new FastBridgeV2(address(this));
fb.grantRole(fb.GOVERNOR_ROLE(), address(this));
fb.addProver(relayer);
return address(fb);
}
Expand Down
1 change: 0 additions & 1 deletion packages/contracts-rfq/test/integration/TokenZapV1.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ abstract contract TokenZapV1IntegrationTest is Test {

function setUp() public virtual {
fastBridge = new FastBridgeV2(address(this));
fastBridge.grantRole(fastBridge.GOVERNOR_ROLE(), address(this));
fastBridge.addProver(relayer);

srcToken = new MockERC20("SRC", 18);
Expand Down

0 comments on commit 2f300e6

Please sign in to comment.