diff --git a/contracts/ScopeGuard.sol b/contracts/ScopeGuard.sol index 1a6e454..c22a776 100644 --- a/contracts/ScopeGuard.sol +++ b/contracts/ScopeGuard.sol @@ -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, @@ -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; } @@ -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. @@ -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. @@ -152,7 +178,7 @@ contract ScopeGuard is FactoryFriendly, BaseGuard { function checkTransaction( address to, - uint256, + uint256 value, bytes memory data, Enum.Operation operation, uint256, @@ -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 || @@ -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" ); } } diff --git a/test/ScopeGuard.spec.ts b/test/ScopeGuard.spec.ts index 0f4e444..a66f0e1 100644 --- a/test/ScopeGuard.spec.ts +++ b/test/ScopeGuard.spec.ts @@ -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, @@ -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, @@ -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, @@ -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( @@ -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 + ) ); }); @@ -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); }); }); @@ -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); }); });