Skip to content

Commit

Permalink
Merge pull request #21 from gnosis/audit-feedback
Browse files Browse the repository at this point in the history 
rename sendAllowed to fallbackAllowed and add valueAllowed
  • Loading branch information
@auryn-macmillan
auryn-macmillan committed Jan 21, 2022
2 parents ed9c85f + f9bf0c3 commit 230f672
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 45 deletions.
55 changes: 44 additions & 11 deletions contracts/ScopeGuard.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import "@gnosis.pm/zodiac/contracts/factory/FactoryFriendly.sol";
contract ScopeGuard is FactoryFriendly, BaseGuard {
event SetTargetAllowed(address target, bool allowed);
event SetTargetScoped(address target, bool scoped);
event SetSendAllowedOnTarget(address target, bool allowed);
event SetFallbackAllowedOnTarget(address target, bool allowed);
event SetValueAllowedOnTarget(address target, bool allowed);
event SetDelegateCallAllowedOnTarget(address target, bool allowed);
event SetFunctionAllowedOnTarget(
address target,
Expand Down Expand Up @@ -36,7 +37,8 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {
bool allowed;
bool scoped;
bool delegateCallAllowed;
bool sendAllowed;
bool fallbackAllowed;
bool valueAllowed;
mapping(bytes4 => bool) allowedFunctions;
}

Expand Down Expand Up @@ -79,12 +81,30 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {
/// @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)
function setFallbackAllowedOnTarget(address target, bool allow)
public
onlyOwner
{
allowedTargets[target].sendAllowed = allow;
emit SetSendAllowedOnTarget(target, allowedTargets[target].sendAllowed);
allowedTargets[target].fallbackAllowed = allow;
emit SetFallbackAllowedOnTarget(
target,
allowedTargets[target].fallbackAllowed
);
}

/// @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 setValueAllowedOnTarget(address target, bool allow)
public
onlyOwner
{
allowedTargets[target].valueAllowed = allow;
emit SetValueAllowedOnTarget(
target,
allowedTargets[target].valueAllowed
);
}

/// @dev Sets whether or not a specific function signature should be allowed on a scoped target.
Expand Down Expand Up @@ -117,10 +137,16 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {
return (allowedTargets[target].scoped);
}

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

/// @dev Returns bool to indicate if ETH can be sent to a target.
/// @param target Address to check.
function isSendAllowed(address target) public view returns (bool) {
return (allowedTargets[target].sendAllowed);
function isValueAllowed(address target) public view returns (bool) {
return (allowedTargets[target].valueAllowed);
}

/// @dev Returns bool to indicate if a function signature is allowed for a target address.
Expand Down Expand Up @@ -152,7 +178,7 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {

function checkTransaction(
address to,
uint256,
uint256 value,
bytes memory data,
Enum.Operation operation,
uint256,
Expand All @@ -170,6 +196,12 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {
"Delegate call not allowed to this address"
);
require(allowedTargets[to].allowed, "Target address is not allowed");
if (value > 0) {
require(
allowedTargets[to].valueAllowed,
"Cannot send ETH to this target"
);
}
if (data.length >= 4) {
require(
!allowedTargets[to].scoped ||
Expand All @@ -179,8 +211,9 @@ contract ScopeGuard is FactoryFriendly, BaseGuard {
} else {
require(data.length == 0, "Function signature too short");
require(
!allowedTargets[to].scoped || allowedTargets[to].sendAllowed,
"Cannot send to this address"
!allowedTargets[to].scoped ||
allowedTargets[to].fallbackAllowed,
"Fallback not allowed for this address"
);
}
}
Expand Down
86 changes: 52 additions & 34 deletions test/ScopeGuard.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,6 @@ describe("ScopeGuard", 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,
Expand All @@ -191,12 +190,11 @@ describe("ScopeGuard", async () => {
).to.be.revertedWith("Function signature too short");
});

it("should revert if scoped and no transaction data is disallowed", async () => {
it("should revert if scoped and empty transaction data is disallowed", async () => {
const { avatar, guard, tx } = await setupTests();
await guard.setTargetAllowed(avatar.address, true);
await guard.setScoped(avatar.address, true);
tx.data = "0x";
tx.value = 1;
await expect(
guard.checkTransaction(
tx.to,
Expand All @@ -211,17 +209,16 @@ describe("ScopeGuard", async () => {
tx.signatures,
user1.address
)
).to.be.revertedWith("Cannot send to this address");
).to.be.revertedWith("Fallback not allowed for this address");
});

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.setFallbackAllowedOnTarget(avatar.address, false);
await guard.setAllowedFunction(avatar.address, "0x00000000", false);
tx.data = "0x00000000";
tx.value = 1;
await expect(
guard.checkTransaction(
tx.to,
Expand All @@ -239,12 +236,10 @@ describe("ScopeGuard", async () => {
).to.be.revertedWith("Target function is not allowed");
});

it("should send to target is send is allowed", async () => {
it("should revert if value is greater than zero and value is not 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.data = "0x12345678";
tx.value = 1;
await expect(
guard.checkTransaction(
Expand All @@ -260,6 +255,29 @@ describe("ScopeGuard", async () => {
tx.signatures,
user1.address
)
).to.be.revertedWith("Cannot send ETH to this target");
});

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

Expand Down Expand Up @@ -410,42 +428,42 @@ describe("ScopeGuard", async () => {
});
});

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

it("should set sendAllowed to true for a target", async () => {
it("should set fallbackAllowed 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")
expect(await guard.isfallbackAllowed(guard.address)).to.be.equals(false);
await expect(guard.setFallbackAllowedOnTarget(guard.address, true))
.to.emit(guard, "SetFallbackAllowedOnTarget")
.withArgs(guard.address, true);
expect(await guard.isSendAllowed(guard.address)).to.be.equals(true);
expect(await guard.isfallbackAllowed(guard.address)).to.be.equals(true);
});

it("should set sendAllowed to false for a target", async () => {
it("should set fallbackAllowed 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")
expect(await guard.isfallbackAllowed(guard.address)).to.be.equals(false);
await expect(guard.setFallbackAllowedOnTarget(guard.address, true))
.to.emit(guard, "SetFallbackAllowedOnTarget")
.withArgs(guard.address, true);
expect(guard.setSendAllowedOnTarget(guard.address, false))
.to.emit(guard, "SetSendAllowedOnTarget")
await expect(guard.setFallbackAllowedOnTarget(guard.address, false))
.to.emit(guard, "SetFallbackAllowedOnTarget")
.withArgs(guard.address, false);
expect(await guard.isSendAllowed(guard.address)).to.be.equals(false);
expect(await guard.isfallbackAllowed(guard.address)).to.be.equals(false);
});

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

expect(guard.setSendAllowedOnTarget(guard.address, true))
.to.emit(guard, "SetSendAllowedOnTarget")
await expect(guard.setFallbackAllowedOnTarget(guard.address, true))
.to.emit(guard, "SetFallbackAllowedOnTarget")
.withArgs(guard.address, true);
});
});
Expand Down Expand Up @@ -536,26 +554,26 @@ describe("ScopeGuard", async () => {
});
});

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

expect(await guard.isSendAllowed(guard.address)).to.be.equals(false);
expect(await guard.isfallbackAllowed(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);
expect(guard.setFallbackAllowedOnTarget(guard.address, false));
expect(await guard.isfallbackAllowed(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);
expect(await guard.isfallbackAllowed(guard.address)).to.be.equals(false);
expect(guard.setFallbackAllowedOnTarget(guard.address, true));
expect(await guard.isfallbackAllowed(guard.address)).to.be.equals(true);
});
});

Expand Down

0 comments on commit 230f672

Please sign in to comment.