Skip to content

Commit

Permalink
Merge pull request #18 from gnosis/fix-acl
Browse files Browse the repository at this point in the history 
Add sendAllow and handle short tx data
  • Loading branch information
@auryn-macmillan
auryn-macmillan committed Nov 16, 2021
2 parents b19af2f + 7e7fe79 commit ed9c85f
Show file tree
Hide file tree
Showing 2 changed files with 169 additions and 15 deletions.
38 changes: 29 additions & 9 deletions contracts/ScopeGuard.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ import "@gnosis.pm/zodiac/contracts/guard/BaseGuard.sol";
import "@gnosis.pm/zodiac/contracts/factory/FactoryFriendly.sol";

contract ScopeGuard is FactoryFriendly, BaseGuard {
event SetTargetAllowed(address target, bool isAllowed);
event SetTargetScoped(address target, bool isScoped);
event SetDelegateCallAllowedOnTarget(address target, bool isAllowed);
event SetTargetAllowed(address target, bool allowed);
event SetTargetScoped(address target, bool scoped);
event SetSendAllowedOnTarget(address target, bool allowed);
event SetDelegateCallAllowedOnTarget(address target, bool allowed);
event SetFunctionAllowedOnTarget(
address target,
bytes4 functionSig,
bool isAllowed
bool allowed
);
event ScopeGuardSetup(address indexed initiator, address indexed owner);

Expand All @@ -35,6 +36,7 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {
bool allowed;
bool scoped;
bool delegateCallAllowed;
bool sendAllowed;
mapping(bytes4 => bool) allowedFunctions;
}

Expand Down Expand Up @@ -67,12 +69,24 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {
/// @dev Sets whether or not calls to an address should be scoped to specific function signatures.
/// @notice Only callable by owner.
/// @param target Address to be scoped/unscoped.
/// @param scope Bool to scope (true) or unscope (false) function calls on target.
function setScoped(address target, bool scope) public onlyOwner {
allowedTargets[target].scoped = scope;
/// @param scoped Bool to scope (true) or unscope (false) function calls on target.
function setScoped(address target, bool scoped) public onlyOwner {
allowedTargets[target].scoped = scoped;
emit SetTargetScoped(target, allowedTargets[target].scoped);
}

/// @dev Sets whether or not a target can be sent to (incluces fallback/receive functions).
/// @notice Only callable by owner.
/// @param target Address to be allow/disallow sends to.
/// @param allow Bool to allow (true) or disallow (false) sends on target.
function setSendAllowedOnTarget(address target, bool allow)
public
onlyOwner
{
allowedTargets[target].sendAllowed = allow;
emit SetSendAllowedOnTarget(target, allowedTargets[target].sendAllowed);
}

/// @dev Sets whether or not a specific function signature should be allowed on a scoped target.
/// @notice Only callable by owner.
/// @param target Scoped address on which a function signature should be allowed/disallowed.
Expand Down Expand Up @@ -103,6 +117,12 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {
return (allowedTargets[target].scoped);
}

/// @dev Returns bool to indicate if allowed to send to a target.
/// @param target Address to check.
function isSendAllowed(address target) public view returns (bool) {
return (allowedTargets[target].sendAllowed);
}

/// @dev Returns bool to indicate if a function signature is allowed for a target address.
/// @param target Address to check.
/// @param functionSig Signature to check.
Expand Down Expand Up @@ -157,9 +177,9 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {
"Target function is not allowed"
);
} else {
require(data.length == 0, "Function signature too short");
require(
!allowedTargets[to].scoped ||
allowedTargets[to].allowedFunctions[bytes4(0)],
!allowedTargets[to].scoped || allowedTargets[to].sendAllowed,
"Cannot send to this address"
);
}
Expand Down
146 changes: 140 additions & 6 deletions test/ScopeGuard.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,28 @@ describe("ScopeGuard", async () => {
).to.be.revertedWith("Target function is not allowed");
});

it("should revert if scoped and transaction data is greater than 0 and less than 4", async () => {
const { avatar, guard, tx } = await setupTests();
await guard.setTargetAllowed(avatar.address, true);
await guard.setScoped(avatar.address, true);
tx.value = 1;
await expect(
guard.checkTransaction(
tx.to,
tx.value,
0x123456,
tx.operation,
tx.avatarTxGas,
tx.baseGas,
tx.gasPrice,
tx.gasToken,
tx.refundReceiver,
tx.signatures,
user1.address
)
).to.be.revertedWith("Function signature too short");
});

it("should revert if scoped and no transaction data is disallowed", async () => {
const { avatar, guard, tx } = await setupTests();
await guard.setTargetAllowed(avatar.address, true);
Expand All @@ -192,7 +214,56 @@ describe("ScopeGuard", async () => {
).to.be.revertedWith("Cannot send to this address");
});

it("it should be callable by a avatar", async () => {
it("should revert if function sig is 0x00000000 and not explicitly approved", async () => {
const { avatar, guard, tx } = await setupTests();
await guard.setTargetAllowed(avatar.address, true);
await guard.setScoped(avatar.address, true);
await guard.setSendAllowedOnTarget(avatar.address, false);
await guard.setAllowedFunction(avatar.address, "0x00000000", false);
tx.data = "0x00000000";
tx.value = 1;
await expect(
guard.checkTransaction(
tx.to,
tx.value,
tx.data,
tx.operation,
tx.avatarTxGas,
tx.baseGas,
tx.gasPrice,
tx.gasToken,
tx.refundReceiver,
tx.signatures,
user1.address
)
).to.be.revertedWith("Target function is not allowed");
});

it("should send to target is send is allowed", async () => {
const { avatar, guard, tx } = await setupTests();
await guard.setTargetAllowed(avatar.address, true);
await guard.setScoped(avatar.address, true);
await guard.setSendAllowedOnTarget(avatar.address, true);
tx.data = "0x";
tx.value = 1;
await expect(
guard.checkTransaction(
tx.to,
tx.value,
tx.data,
tx.operation,
tx.avatarTxGas,
tx.baseGas,
tx.gasPrice,
tx.gasToken,
tx.refundReceiver,
tx.signatures,
user1.address
)
);
});

it("should be callable by an avatar", async () => {
const { avatar, guard, tx } = await setupTests();
expect(guard.setTargetAllowed(guard.address, true));
tx.operation = 0;
Expand Down Expand Up @@ -339,6 +410,46 @@ describe("ScopeGuard", async () => {
});
});

describe("setSendAllowedOnTarget()", async () => {
it("should revert if caller is not owner", async () => {
const { guard } = await setupTests();
expect(
guard.connect(user2).setSendAllowedOnTarget(guard.address, true)
).to.be.revertedWith("caller is not the owner");
});

it("should set sendAllowed to true for a target", async () => {
const { guard } = await setupTests();

expect(await guard.isSendAllowed(guard.address)).to.be.equals(false);
expect(guard.setSendAllowedOnTarget(guard.address, true))
.to.emit(guard, "SetSendAllowedOnTarget")
.withArgs(guard.address, true);
expect(await guard.isSendAllowed(guard.address)).to.be.equals(true);
});

it("should set sendAllowed to false for a target", async () => {
const { guard } = await setupTests();

expect(await guard.isSendAllowed(guard.address)).to.be.equals(false);
expect(guard.setSendAllowedOnTarget(guard.address, true))
.to.emit(guard, "SetSendAllowedOnTarget")
.withArgs(guard.address, true);
expect(guard.setSendAllowedOnTarget(guard.address, false))
.to.emit(guard, "SetSendAllowedOnTarget")
.withArgs(guard.address, false);
expect(await guard.isSendAllowed(guard.address)).to.be.equals(false);
});

it("should emit SetSendAllowedOnTarget(target, scoped)", async () => {
const { guard } = await setupTests();

expect(guard.setSendAllowedOnTarget(guard.address, true))
.to.emit(guard, "SetSendAllowedOnTarget")
.withArgs(guard.address, true);
});
});

describe("setAllowedFunction()", async () => {
it("should revert if caller is not owner", async () => {
const { guard } = await setupTests();
Expand Down Expand Up @@ -386,7 +497,7 @@ describe("ScopeGuard", async () => {
});
});

describe("isAllowedTarget", async () => {
describe("isAllowedTarget()", async () => {
it("should return false if not set", async () => {
const { avatar, guard } = await setupTests();

Expand All @@ -402,7 +513,7 @@ describe("ScopeGuard", async () => {
});
});

describe("isScoped", async () => {
describe("isScoped()", async () => {
it("should return false if not set", async () => {
const { avatar, guard } = await setupTests();

Expand All @@ -425,7 +536,30 @@ describe("ScopeGuard", async () => {
});
});

describe("isAllowedFunction", async () => {
describe("isSendAllowed()", async () => {
it("should return false if not set", async () => {
const { avatar, guard } = await setupTests();

expect(await guard.isSendAllowed(guard.address)).to.be.equals(false);
});

it("should return false if set to false", async () => {
const { guard } = await setupTests();

expect(guard.setSendAllowedOnTarget(guard.address, false));
expect(await guard.isSendAllowed(guard.address)).to.be.equals(false);
});

it("should return true if set to true", async () => {
const { guard } = await setupTests();

expect(await guard.isSendAllowed(guard.address)).to.be.equals(false);
expect(guard.setSendAllowedOnTarget(guard.address, true));
expect(await guard.isSendAllowed(guard.address)).to.be.equals(true);
});
});

describe("isAllowedFunction()", async () => {
it("should return false if not set", async () => {
const { avatar, guard } = await setupTests();

Expand All @@ -449,8 +583,8 @@ describe("ScopeGuard", async () => {
});
});

describe("isAllowedToDelegateCall", async () => {
it("should return false by default", async () => {
describe("isAllowedToDelegateCall()", async () => {
it("should return false if not set", async () => {
const { avatar, guard } = await setupTests();

expect(await guard.isAllowedTarget(avatar.address)).to.be.equals(false);
Expand Down

0 comments on commit ed9c85f

Please sign in to comment.