Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change library functions to internal #8

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

jonwalch
Copy link
Contributor

change some library function from public to internal to account for foundry-rs/foundry#4266 (comment)

change some library function from public to internal to account for foundry-rs/foundry#4266 (comment)
@colinnielsen colinnielsen marked this pull request as ready for review July 27, 2023 08:51
@colinnielsen
Copy link
Owner

I think this is probably the right approach @jonwalch.

I didn't consider how public library functions could affect custom vm.prank calls.
FWIW, this could also be solved with the SafeInstance being it's own contract, but I personally like that pattern of putting the using _ for _; directive at the top of the test, which attaches the methods to the struct - what are your thoughts?

Just for my sake, do you mind including a snippet of the code which caused this original issue?

@jonwalch
Copy link
Contributor Author

// SPDX-License-Identifier: ISC
pragma solidity ^0.8.19;

// ====================================================================
// |     ______                   _______                             |
// |    / _____________ __  __   / ____(_____  ____ _____  ________   |
// |   / /_  / ___/ __ `| |/_/  / /_  / / __ \/ __ `/ __ \/ ___/ _ \  |
// |  / __/ / /  / /_/ _>  <   / __/ / / / / / /_/ / / / / /__/  __/  |
// | /_/   /_/   \__,_/_/|_|  /_/   /_/_/ /_/\__,_/_/ /_/\___/\___/   |
// |                                                                  |
// ====================================================================
// ============================ FraxGuard =============================
// ====================================================================
// Frax Finance: https://github.com/FraxFinance

// Primary Author(s)
// Jon Walch: https://github.com/jonwalch

// Reviewers
// Drake Evans: https://github.com/DrakeEvans
// Dennis: https://github.com/denett
// Sam Kazemian: https://github.com/samkazemian

// ====================================================================

import { IERC165 } from "@gnosis.pm/contracts/interfaces/IERC165.sol";
import { Guard } from "@gnosis.pm/contracts/base/GuardManager.sol";
import { Enum, ISafe } from "./interfaces/ISafe.sol";

/// @title FraxGuard
/// @author Jon Walch (Frax Finance) https://github.com/jonwalch
/// @notice  A Gnosis Safe Guard that restricts Safe transaction execution to Safe owners and requires approval from FraxGovernorOmega
contract FraxGuard is IERC165, Guard {
    /// @notice The address of the FraxGovernorOmega contract
    address public immutable FRAX_GOVERNOR_OMEGA;

    /// @notice The ```constructor``` function is called on deployment
    /// @param fraxGovernorOmega The address of the FraxGovernorOmega contract
    constructor(address fraxGovernorOmega) {
        FRAX_GOVERNOR_OMEGA = fraxGovernorOmega;
    }

    /// @notice The ```checkTransaction``` function is a "callback" from within GnosisSafe::execTransaction() that runs before execution
    /// @param to Destination address of Safe transaction.
    /// @param value Ether value of Safe transaction.
    /// @param data Data payload of Safe transaction.
    /// @param operation Operation type of Safe transaction.
    /// @param safeTxGas Gas that should be used for the Safe transaction.
    /// @param baseGas Gas costs that are independent of the transaction execution(e.g. base transaction fee, signature check, payment of the refund)
    /// @param gasPrice Gas price that should be used for the payment calculation.
    /// @param gasToken Token address (or 0 if ETH) that is used for the payment.
    /// @param refundReceiver Address of receiver of gas payment (or 0 if tx.origin).
    /// @param msgSender Address of caller of GnosisSafe::execTransaction()
    function checkTransaction(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation,
        uint256 safeTxGas,
        uint256 baseGas,
        uint256 gasPrice,
        address gasToken,
        address payable refundReceiver,
        bytes memory, // signatures Packed signature data ({bytes32 r}{bytes32 s}{uint8 v})
        address msgSender
    ) external {
        ISafe safe = ISafe(msg.sender);
        bytes32 txHash = safe.getTransactionHash({
            to: to,
            value: value,
            data: data,
            operation: operation,
            safeTxGas: safeTxGas,
            baseGas: baseGas,
            gasPrice: gasPrice,
            gasToken: gasToken,
            refundReceiver: refundReceiver,
            _nonce: safe.nonce() - 1 // nonce gets incremented before this function is called
        });
        if (!safe.isOwner(msgSender) || safe.approvedHashes({ signer: FRAX_GOVERNOR_OMEGA, txHash: txHash }) != 1) {
            revert Unauthorized();
        }
    }

    /// @notice The ```checkAfterExecution``` function is a "callback" from within GnosisSafe::execTransaction() that runs after execution
    function checkAfterExecution(bytes32, /* txHash */ bool /* success */) external {}

    function supportsInterface(bytes4 interfaceId) external view virtual override returns (bool) {
        return
            interfaceId == type(Guard).interfaceId || // 0xe6d7a83a
            interfaceId == type(IERC165).interfaceId; // 0x01ffc9a7
    }

    error Unauthorized();
}
    function testReproMsgSender() public {
        // We have a custom guard outlined above that restricts who can call execTransaction based on msgSender

        // with:
        //  function EIP1271Sign(SafeInstance memory instance, bytes32 digest) public {
        //      EIP1271Sign(instance, abi.encodePacked(digest));
        //  }
        // GnosisSafeProxy::isOwner(TestFraxGovernor: [0x90193C961A926261B756D1E5bb255e67ff9498A1])
        // wrong msgSender, this test file

        // with:
        //  function EIP1271Sign(SafeInstance memory instance, bytes32 digest) internal {
        //      EIP1271Sign(instance, abi.encodePacked(digest));
        //  }
        // GnosisSafeProxy::isOwner(: [0x1a376cFe2c4d4BA53843a1Dc5f9c0758C51c4F37])
        // correct msgSender, eoaOwners[0]

        bytes32 digest;

        vm.startPrank(eoaOwners[0]);
        SafeTestLib.EIP1271Sign(getSafe(address(multisig)), digest);
        vm.stopPrank();
    }

@colinnielsen colinnielsen merged commit b2baf47 into colinnielsen:main Nov 7, 2023
@jonwalch jonwalch changed the title WIP change library functions to internal Change library functions to internal Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants