Skip to content

Commit

Permalink
[#97] Create a function for checking permissions
Browse files Browse the repository at this point in the history
  • Loading branch information
@akshay-ap
akshay-ap committed Sep 8, 2023
1 parent 8de6bc5 commit 4ce3319
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 30 deletions.
32 changes: 17 additions & 15 deletions contracts/SafeProtocolManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,8 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana
event PluginDisabled(address indexed safe, address indexed plugin);

// Errors
error PluginRequiresRootAccess(address sender);
error PluginNotEnabled(address plugin);
error PluginEnabledOnlyForRootAccess(address plugin);
error MissingPluginPermission(address plugin, uint8 pluginRequires, uint8 requiredPermission, uint8 givenPermission);
error PluginPermissionsMismatch(address plugin, uint8 requiredPermissions, uint8 givenPermissions);
error ActionExecutionFailed(address safe, bytes32 metadataHash, uint256 index);
error RootAccessActionExecutionFailed(address safe, bytes32 metadataHash);
Expand Down Expand Up @@ -93,13 +92,12 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana
for (uint256 i = 0; i < length; ++i) {
SafeProtocolAction calldata safeProtocolAction = transaction.actions[i];

if (
safeProtocolAction.to == address(this) ||
(safeProtocolAction.to == safeAddress &&
(enabledPlugins[safeAddress][msg.sender].permissions & PLUGIN_PERMISSION_CALL_TO_SELF !=
PLUGIN_PERMISSION_CALL_TO_SELF))
) {
if (safeProtocolAction.to == address(this)) {
revert InvalidToFieldInSafeProtocolAction(safeAddress, transaction.metadataHash, i);
} else if (safeProtocolAction.to == safeAddress) {
checkPermission(safeAddress, PLUGIN_PERMISSION_CALL_TO_SELF);
} else {
checkPermission(safeAddress, PLUGIN_PERMISSION_EXECUTE_CALL);
}

(bool isActionSuccessful, bytes memory resultData) = safe.execTransactionFromModuleReturnData(
Expand Down Expand Up @@ -146,13 +144,8 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana
// executionType = 1 for plugin flow
preCheckData = ISafeProtocolHooks(hooksAddress).preCheckRootAccess(safe, rootAccess, 1, abi.encode(msg.sender));
}
if (
((ISafeProtocolPlugin(msg.sender).requiresPermissions() &
(enabledPlugins[safeAddress][msg.sender].permissions & PLUGIN_PERMISSION_EXECUTE_DELEGATECALL)) !=
PLUGIN_PERMISSION_EXECUTE_DELEGATECALL)
) {
revert PluginRequiresRootAccess(msg.sender);
}

checkPermission(safeAddress, PLUGIN_PERMISSION_EXECUTE_DELEGATECALL);

bool success;
(success, data) = safe.execTransactionFromModuleReturnData(
Expand Down Expand Up @@ -453,4 +446,13 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana
revert InvalidPluginAddress(plugin);
}
}

function checkPermission(address account, uint8 permission) private view {
uint8 givenPermissions = enabledPlugins[account][msg.sender].permissions;
uint8 requiresPermissions = ISafeProtocolPlugin(msg.sender).requiresPermissions();

if ((requiresPermissions & givenPermissions & permission) != permission) {
revert MissingPluginPermission(msg.sender, requiresPermissions, permission, givenPermissions);
}
}
}
45 changes: 30 additions & 15 deletions test/SafeProtocolManager.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ describe("SafeProtocolManager", async () => {
await expect(tx).to.emit(safeProtocolManager, "ActionsExecuted").withArgs(safeAddress, safeTx.metadataHash, 1);
});

it("Should revert with a InvalidToFieldInSafeProtocolAction when `to` address is safe address through which tx is executed", async function () {
it("Should revert with MissingPluginPermission when `to` address is account and plugin does not have CALL_TO_SELF permission", async function () {
const { safeProtocolManager, safeProtocolRegistry, safe } = await loadFixture(deployContractsWithEnabledManagerFixture);

// Enable plugin
Expand All @@ -570,12 +570,18 @@ describe("SafeProtocolManager", async () => {
PLUGIN_PERMISSION_EXECUTE_CALL,
]);
await safe.exec(safe.target, 0, data);
const safeAddress = await safe.getAddress();
const safeTx = buildSingleTx(safeAddress, hre.ethers.parseEther("1"), "0x", BigInt(1), hre.ethers.randomBytes(32));
await expect(plugin.executeFromPlugin(safeProtocolManager, safe, safeTx)).to.be.revertedWithCustomError(
safeProtocolManager,
"InvalidToFieldInSafeProtocolAction",
);

await plugin.setRequiresPermissions(PLUGIN_PERMISSION_CALL_TO_SELF);

const safeTx = buildSingleTx(safe.target, hre.ethers.parseEther("1"), "0x", BigInt(1), hre.ethers.randomBytes(32));
await expect(plugin.executeFromPlugin(safeProtocolManager, safe, safeTx))
.to.be.revertedWithCustomError(safeProtocolManager, "MissingPluginPermission")
.withArgs(
plugin.target,
PLUGIN_PERMISSION_CALL_TO_SELF,
PLUGIN_PERMISSION_CALL_TO_SELF,
PLUGIN_PERMISSION_EXECUTE_CALL,
);
});

it("Should revert with a InvalidToFieldInSafeProtocolAction when `to` address is Manager address", async function () {
Expand Down Expand Up @@ -1029,7 +1035,7 @@ describe("SafeProtocolManager", async () => {
);
});

it("Should revert with PluginRequiresRootAccess if plugin indicates it doesn't need root access anymore", async () => {
it("Should revert with PluginPermissionsMismatch if plugin indicates it doesn't need root access anymore", async () => {
const { safeProtocolManager, safe, safeProtocolRegistry } = await loadFixture(deployContractsWithEnabledManagerFixture);

const testFallbackReceiver = await (await hre.ethers.getContractFactory("TestFallbackReceiver")).deploy(user1.address);
Expand All @@ -1053,10 +1059,14 @@ describe("SafeProtocolManager", async () => {
hre.ethers.randomBytes(32),
);

await expect(plugin.executeRootAccessTxFromPlugin(safeProtocolManager, safe, safeTx)).to.be.revertedWithCustomError(
safeProtocolManager,
"PluginRequiresRootAccess",
);
await expect(plugin.executeRootAccessTxFromPlugin(safeProtocolManager, safe, safeTx))
.to.be.revertedWithCustomError(safeProtocolManager, "MissingPluginPermission")
.withArgs(
plugin.target,
PLUGIN_PERMISSION_EXECUTE_CALL,
PLUGIN_PERMISSION_DELEGATE_CALL,
PLUGIN_PERMISSION_DELEGATE_CALL,
);
});

it("Should emit RootAccessActionExecutionFailed when root access action execution fails", async () => {
Expand Down Expand Up @@ -1087,7 +1097,7 @@ describe("SafeProtocolManager", async () => {
);
});

it("Should emit PluginRequiresRootAccess for root access plugin", async () => {
it("Should emit MissingPluginPermission when plugin is not granted root access", async () => {
const { safeProtocolManager, safe, safeProtocolRegistry } = await loadFixture(deployContractsWithEnabledManagerFixture);

const testFallbackReceiver = await (await hre.ethers.getContractFactory("TestFallbackReceiverReverter")).deploy();
Expand Down Expand Up @@ -1115,8 +1125,13 @@ describe("SafeProtocolManager", async () => {
hre.ethers.randomBytes(32),
);
await expect(plugin.executeRootAccessTxFromPlugin(safeProtocolManager, safe, safeTx))
.to.be.revertedWithCustomError(safeProtocolManager, "PluginRequiresRootAccess")
.withArgs(pluginAddress);
.to.be.revertedWithCustomError(safeProtocolManager, "MissingPluginPermission")
.withArgs(
pluginAddress,
PLUGIN_PERMISSION_DELEGATE_CALL,
PLUGIN_PERMISSION_DELEGATE_CALL,
PLUGIN_PERMISSION_EXECUTE_CALL,
);
});
});
});
Expand Down

0 comments on commit 4ce3319

Please sign in to comment.