diff --git a/.solcover.js b/.solcover.js index 6db86260d..0e6c5a1d7 100644 --- a/.solcover.js +++ b/.solcover.js @@ -1,6 +1,8 @@ module.exports = { skipFiles: [ 'test/Token.sol', + 'test/ERC20Token.sol', + 'test/TestHandler.sol', 'test/ERC1155Token.sol', ], mocha: { diff --git a/contracts/base/FallbackManager.sol b/contracts/base/FallbackManager.sol index 48cca08c1..ee16acc32 100644 --- a/contracts/base/FallbackManager.sol +++ b/contracts/base/FallbackManager.sol @@ -47,7 +47,11 @@ contract FallbackManager is SelfAuthorized { // solium-disable-next-line security/no-inline-assembly assembly { calldatacopy(0, 0, calldatasize()) - let success := call(gas, handler, 0, 0, calldatasize(), 0, 0) + // The msg.sender address is shifted to the left by 12 bytes to remove the padding + // Then the address without padding is stored right after the calldata + mstore(calldatasize(), shl(96, caller())) + // Add 20 bytes for the address appended add the end + let success := call(gas, handler, 0, 0, add(calldatasize(), 20), 0, 0) returndatacopy(0, 0, returndatasize()) if eq(success, 0) { revert(0, returndatasize()) } return(0, returndatasize()) diff --git a/contracts/handler/HandlerContext.sol b/contracts/handler/HandlerContext.sol new file mode 100644 index 000000000..805d2f05d --- /dev/null +++ b/contracts/handler/HandlerContext.sol @@ -0,0 +1,19 @@ +pragma solidity >=0.5.0 <0.6.0; + +/// @title Handler Context - allows to extract calling context +/// @author Richard Meissner - +/// @notice based on https://github.com/OpenZeppelin/openzeppelin-contracts/blob/f8cc8b844a9f92f63dc55aa581f7d643a1bc5ac1/contracts/metatx/ERC2771Context.sol +contract HandlerContext { + + // This function does not rely on a trusted forwarder. Use the returned value only to check information against the calling manager. + function _msgSender() internal pure returns (address sender) { + // The assembly code is more direct than the Solidity version using `abi.decode`. + assembly { sender := shr(96, calldataload(sub(calldatasize(), 20))) } + } + + // Function do differentiate more clearly between msg.sender and the calling manager + function _manager() internal view returns (address) { + return msg.sender; + } + +} \ No newline at end of file diff --git a/contracts/test/TestHandler.sol b/contracts/test/TestHandler.sol new file mode 100644 index 000000000..94b7219a4 --- /dev/null +++ b/contracts/test/TestHandler.sol @@ -0,0 +1,7 @@ +pragma solidity >=0.5.0 <0.6.0; +import "../handler/HandlerContext.sol"; +contract TestHandler is HandlerContext { + function dudududu() external returns (address sender, address manager) { + return (_msgSender(), _manager()); + } +} diff --git a/test/core/GnosisSafe.FallbackManager.spec.ts b/test/core/GnosisSafe.FallbackManager.spec.ts index cd223a404..b101bfd58 100644 --- a/test/core/GnosisSafe.FallbackManager.spec.ts +++ b/test/core/GnosisSafe.FallbackManager.spec.ts @@ -3,21 +3,35 @@ import hre, { deployments, waffle } from "hardhat"; import { BigNumber } from "ethers"; import "@nomiclabs/hardhat-ethers"; import { AddressZero } from "@ethersproject/constants"; -import { defaultCallbackHandlerContract, defaultCallbackHandlerDeployment, getSafeTemplate } from "../utils/setup"; +import { defaultCallbackHandlerContract, defaultCallbackHandlerDeployment, deployContract, getMock, getSafeTemplate } from "../utils/setup"; import { executeContractCallWithSigners } from "../utils/execution"; describe("FallbackManager", async () => { const setupWithTemplate = deployments.createFixture(async ({ deployments }) => { await deployments.fixture(); - return await getSafeTemplate() + const source = ` + contract Mirror { + function lookAtMe() public returns (bytes memory) { + return msg.data; + } + + function nowLookAtYou(address you, string memory howYouLikeThat) public returns (bytes memory) { + return msg.data; + } + }` + const mirror = await deployContract(user1, source); + return { + safe: await getSafeTemplate(), + mirror + } }) const [user1, user2] = waffle.provider.getWallets(); describe("setFallbackManager", async () => { it('is correctly set on deployment', async () => { - const safe = await setupWithTemplate() + const { safe } = await setupWithTemplate() const handler = await defaultCallbackHandlerDeployment() // Check fallback handler @@ -36,7 +50,7 @@ describe("FallbackManager", async () => { }) it('is correctly set', async () => { - const safe = await setupWithTemplate() + const { safe } = await setupWithTemplate() const handler = await defaultCallbackHandlerDeployment() // Setup Safe @@ -57,7 +71,7 @@ describe("FallbackManager", async () => { }) it('is called when set', async () => { - const safe = await setupWithTemplate() + const { safe } = await setupWithTemplate() const handler = await defaultCallbackHandlerDeployment() const safeHandler = (await defaultCallbackHandlerContract()).attach(safe.address) // Check that Safe is NOT setup @@ -76,5 +90,49 @@ describe("FallbackManager", async () => { await safeHandler.callStatic.onERC1155Received(AddressZero, AddressZero, 0, 0, "0x") ).to.be.eq("0xf23a6e61") }) + + it('sends along msg.sender on simple call', async () => { + const { safe, mirror } = await setupWithTemplate() + // Setup Safe + await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", mirror.address, AddressZero, 0, AddressZero) + + const tx = { + to: safe.address, + data: mirror.interface.encodeFunctionData("lookAtMe") + } + // Check that mock works as handler + const response = await user1.call(tx) + expect(response).to.be.eq( + "0x" + + "0000000000000000000000000000000000000000000000000000000000000020" + + "0000000000000000000000000000000000000000000000000000000000000018" + + "7f8dc53c" + user1.address.slice(2).toLowerCase() + "0000000000000000" + ) + }) + + it('sends along msg.sender on more complex call', async () => { + const { safe, mirror } = await setupWithTemplate() + // Setup Safe + await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", mirror.address, AddressZero, 0, AddressZero) + + const tx = { + to: safe.address, + data: mirror.interface.encodeFunctionData("nowLookAtYou", [user2.address, "pink<>black"]) + } + // Check that mock works as handler + const response = await user1.call(tx) + expect(response).to.be.eq( + "0x" + + "0000000000000000000000000000000000000000000000000000000000000020" + + "0000000000000000000000000000000000000000000000000000000000000098" + + // Function call + "b2a88d99" + + "000000000000000000000000" + user2.address.slice(2).toLowerCase() + + "0000000000000000000000000000000000000000000000000000000000000040" + + "000000000000000000000000000000000000000000000000000000000000000b" + + "70696e6b3c3e626c61636b000000000000000000000000000000000000000000" + + user1.address.slice(2).toLowerCase() + "0000000000000000" + ) + }) }) }) \ No newline at end of file diff --git a/test/handlers/HandlerContext.spec.ts b/test/handlers/HandlerContext.spec.ts new file mode 100644 index 000000000..8ac06f281 --- /dev/null +++ b/test/handlers/HandlerContext.spec.ts @@ -0,0 +1,45 @@ +import { expect } from "chai"; +import hre, { deployments, waffle } from "hardhat"; +import "@nomiclabs/hardhat-ethers"; +import { AddressZero } from "@ethersproject/constants"; +import { getSafeTemplate } from "../utils/setup"; + +describe("HandlerContext", async () => { + + const [user1, user2] = waffle.provider.getWallets(); + + const setup = deployments.createFixture(async ({ deployments }) => { + await deployments.fixture(); + const TestHandler = await hre.ethers.getContractFactory("TestHandler"); + const handler = await TestHandler.deploy(); + return { + safe: await getSafeTemplate(), + handler + } + }) + + it('parses information correctly', async () => { + const { handler } = await setup(); + const response = await user1.call({ + to: handler.address, + data: handler.interface.encodeFunctionData("dudududu") + user2.address.slice(2) + }) + expect( + handler.interface.decodeFunctionResult("dudududu", response) + ).to.be.deep.eq([user2.address, user1.address]) + }) + + it('works with the Safe', async () => { + const { safe, handler } = await setup(); + await safe.setup([user1.address, user2.address], 1, AddressZero, "0x", handler.address, AddressZero, 0, AddressZero) + + const response = await user1.call({ + to: safe.address, + data: handler.interface.encodeFunctionData("dudududu") + }) + + expect( + handler.interface.decodeFunctionResult("dudududu", response) + ).to.be.deep.eq([user1.address, safe.address]) + }) +}) \ No newline at end of file