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

Fix/audits 4 #70

Merged
merged 9 commits into from
Dec 23, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/deployFunnelForToken.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
152535
152523
Original file line number Diff line number Diff line change
@@ -1 +1 @@
108767
108878
2 changes: 1 addition & 1 deletion .forge-snapshots/executeMetaTransactionTransfer.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
75914
76024
32 changes: 16 additions & 16 deletions .gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ FunnelTest:testApproveRenewableAndCallWithGasSnapshot() (gas: 121012)
FunnelTest:testBaseToken() (gas: 9905)
FunnelTest:testBaseTokenAllowance() (gas: 103961)
FunnelTest:testClearRenewableAllowanceOnNormalApprove() (gas: 143323)
FunnelTest:testExecuteMetaTransactionApproveRenewable() (gas: 211775)
FunnelTest:testExecuteMetaTransactionTransfer() (gas: 127828)
FunnelTest:testExecuteMetaTransactionApproveRenewable() (gas: 211886)
FunnelTest:testExecuteMetaTransactionTransfer() (gas: 127938)
FunnelTest:testFallbackToBaseToken() (gas: 25840)
FunnelTest:testInfiniteApproveTransferFrom() (gas: 132794)
FunnelTest:testInsufficientAllowance() (gas: 21475)
Expand All @@ -17,7 +17,7 @@ FunnelTest:testOverflow() (gas: 115155)
FunnelTest:testOverflow2() (gas: 115176)
FunnelTest:testOverflow3() (gas: 128616)
FunnelTest:testOverriddenName() (gas: 20952)
FunnelTest:testOverriddenNameWithNoName() (gas: 2045242)
FunnelTest:testOverriddenNameWithNoName() (gas: 1990130)
FunnelTest:testPermit() (gas: 159444)
FunnelTest:testPermitRenewable() (gas: 130905)
FunnelTest:testPermitRenewableContractWallet() (gas: 126685)
Expand All @@ -26,16 +26,16 @@ FunnelTest:testRecoveryRateExceeded() (gas: 13452)
FunnelTest:testRecoveryRateExceeded2() (gas: 13492)
FunnelTest:testRenewableAllowanceTransferFrom() (gas: 149827)
FunnelTest:testRenewableMaxAllowance() (gas: 98365)
FunnelTest:testRevertExecuteMetaTransactionBadNonce() (gas: 48705)
FunnelTest:testRevertExecuteMetaTransactionCallFailed() (gas: 55277)
FunnelTest:testRevertExecuteMetaTransactionInvalidSigner() (gas: 41411)
FunnelTest:testRevertExecuteMetaTransactionReplayProtection() (gas: 144440)
FunnelTest:testRevertExecuteMetaTransactionBadNonce() (gas: 48729)
FunnelTest:testRevertExecuteMetaTransactionCallFailed() (gas: 55301)
FunnelTest:testRevertExecuteMetaTransactionInvalidSigner() (gas: 41435)
FunnelTest:testRevertExecuteMetaTransactionReplayProtection() (gas: 144575)
FunnelTest:testRevertPermitBadDeadline() (gas: 47105)
FunnelTest:testRevertPermitBadNonce() (gas: 47002)
FunnelTest:testRevertPermitPastDeadline() (gas: 16239)
FunnelTest:testRevertPermitRenewableBadDeadline() (gas: 49183)
FunnelTest:testRevertPermitRenewableBadNonce() (gas: 49146)
FunnelTest:testRevertPermitRenewableContractWalletBadSigner() (gas: 52942)
FunnelTest:testRevertPermitRenewableContractWalletBadSigner() (gas: 52922)
FunnelTest:testRevertPermitRenewablePastDeadline() (gas: 20595)
FunnelTest:testRevertPermitRenewableReplay() (gas: 126066)
FunnelTest:testRevertPermitReplay() (gas: 123961)
Expand All @@ -48,7 +48,7 @@ FunnelTest:testTransferFromAndCallRevertNonContract() (gas: 112100)
FunnelTest:testTransferFromAndCallRevertNonReceiver() (gas: 110453)
FunnelTest:testTransferFromAndCallWithGasSnapshot() (gas: 172942)
FunnelTest:testTransferFromWithSnapshot() (gas: 155381)
FunnelTest:testZeroAddressInitialise() (gas: 1787778)
FunnelTest:testZeroAddressInitialise() (gas: 1732666)
FunnelERC20Test:testApproveFuzzing(uint256) (runs: 256, μ: 117723, ~: 119900)
FunnelERC20Test:testApproveMaxWithTransfer() (gas: 137307)
FunnelERC20Test:testApproveWithTransferFuzzing(uint256,uint256) (runs: 256, μ: 147561, ~: 149022)
Expand All @@ -67,16 +67,16 @@ FunnelERC20Test:testTransferFuzzing(uint256) (runs: 256, μ: 68303, ~: 69678)
FunnelERC20Test:testTransferHalfBalance() (gas: 66255)
FunnelERC20Test:testTransferOneToken() (gas: 64089)
FunnelERC20Test:testTransferZeroTokens() (gas: 41433)
FunnelFactoryTest:testDeployFunnelForDifferentTokens() (gas: 298301)
FunnelFactoryTest:testDeployFunnelForToken() (gas: 190140)
FunnelFactoryTest:testDeployFunnelForTokenRevertsIfAlreadyDeployed() (gas: 156740)
FunnelFactoryTest:testDeployFunnelFromDifferentFactory() (gas: 558192)
FunnelFactoryTest:testDeployFunnelForDifferentTokens() (gas: 298277)
FunnelFactoryTest:testDeployFunnelForToken() (gas: 190128)
FunnelFactoryTest:testDeployFunnelForTokenRevertsIfAlreadyDeployed() (gas: 156728)
FunnelFactoryTest:testDeployFunnelFromDifferentFactory() (gas: 557162)
FunnelFactoryTest:testGetFunnelForTokenRevertsIfNotDeployed() (gas: 12826)
FunnelFactoryTest:testIsFunnelFalseForDeployedFunnelFromDifferentFactory() (gas: 563344)
FunnelFactoryTest:testIsFunnelFalseForDeployedFunnelFromDifferentFactory() (gas: 562314)
FunnelFactoryTest:testIsFunnelFalseForNonFunnel() (gas: 10897)
FunnelFactoryTest:testIsFunnelFalseForUndeployedFunnel() (gas: 8329)
FunnelFactoryTest:testIsFunnelTrueForDeployedFunnel() (gas: 155345)
FunnelFactoryTest:testIsFunnelTrueForDeployedFunnel() (gas: 155333)
FunnelFactoryTest:testNoCodeTokenReverts() (gas: 15454)
FunnelFactoryTest:testTransferFromFunnel() (gas: 291706)
FunnelFactoryTest:testTransferFromFunnel() (gas: 291694)
MockSpenderReceiverTest:testSupportInterfaceReceiver() (gas: 5598)
MockSpenderReceiverTest:testSupportInterfaceSpender() (gas: 6567)
13 changes: 6 additions & 7 deletions src/Funnel.sol
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable,
}

// owner => spender => renewableAllowance
mapping(address => mapping(address => RenewableAllowance)) rAllowance;
mapping(address => mapping(address => RenewableAllowance)) private rAllowance;
zlace0x marked this conversation as resolved.
Show resolved Hide resolved

//////////////////////////////////////////////////////////////
/// EIP-2612 STORAGE
Expand All @@ -70,8 +70,7 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable,
bytes32 internal constant PERMIT_TYPEHASH =
keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");

/// @notice Called when the contract is being initialised.
/// @dev Sets the INITIAL_CHAIN_ID and INITIAL_DOMAIN_SEPARATOR that might be used in future permit calls
/// @inheritdoc IFunnel
function initialize(address _token) external initializer {
if (_token == address(0)) {
revert InvalidAddress({ _input: _token });
Expand Down Expand Up @@ -292,7 +291,7 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable,
/// @param interfaceId The interface identifier, as specified in ERC-165
/// @dev Interface identification is specified in ERC-165. See https://eips.ethereum.org/EIPS/eip-165
/// @return `true` if the contract implements `interfaceID`
function supportsInterface(bytes4 interfaceId) external pure virtual returns (bool) {
function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
return
interfaceId == type(IERC5827).interfaceId ||
interfaceId == type(IERC5827Payable).interfaceId ||
Expand All @@ -308,7 +307,7 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable,
/// This is a low level function that doesn't return to its internal call site.
/// It will return to the external caller whatever the implementation returns.
/// @param implementation Address to delegate.
function _fallback(address implementation) internal virtual {
function _fallback(address implementation) internal view {
assembly {
// Copy msg.data. We take full control of memory in this inline assembly
// block because it will not return to Solidity code. We overwrite the
Expand All @@ -323,7 +322,7 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable,
returndatacopy(0, 0, returndatasize())

switch result
// delegatecall returns 0 on error.
// staticcall returns 0 on error.
case 0 {
revert(0, returndatasize())
}
Expand Down Expand Up @@ -451,7 +450,7 @@ contract Funnel is IFunnel, NativeMetaTransaction, MetaTxContext, Initializable,
/// @notice compute the domain seperator that is required for the approve by signature functionality
/// Stops replay attacks from happening because of approvals on different contracts on different chains
/// @dev Reference https://eips.ethereum.org/EIPS/eip-712
function _computeDomainSeparator() internal view virtual returns (bytes32) {
function _computeDomainSeparator() internal view returns (bytes32) {
return
keccak256(
abi.encode(
Expand Down
5 changes: 3 additions & 2 deletions src/FunnelFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { IFunnelFactory } from "./interfaces/IFunnelFactory.sol";
import { IERC5827Proxy } from "./interfaces/IERC5827Proxy.sol";
import { IFunnelErrors } from "./interfaces/IFunnelErrors.sol";
import { Funnel } from "./Funnel.sol";
import { IFunnel } from "./interfaces/IFunnel.sol";
import { Clones } from "openzeppelin-contracts/proxy/Clones.sol";

/// @title Factory for all the funnel contracts
Expand All @@ -13,7 +14,7 @@ contract FunnelFactory is IFunnelFactory, IFunnelErrors {
using Clones for address;

/// Stores the mapping between tokenAddress => funnelAddress
mapping(address => address) deployments;
mapping(address => address) private deployments;

/// address of the implementation. This is immutable due to security as implementation is not
/// supposed to change after deployment
Expand Down Expand Up @@ -42,8 +43,8 @@ contract FunnelFactory is IFunnelFactory, IFunnelErrors {
_funnelAddress = funnelImplementation.cloneDeterministic(bytes32(uint256(uint160(_tokenAddress))));

deployments[_tokenAddress] = _funnelAddress;
Funnel(_funnelAddress).initialize(_tokenAddress);
emit DeployedFunnel(_tokenAddress, _funnelAddress);
IFunnel(_funnelAddress).initialize(_tokenAddress);
}

/// @inheritdoc IFunnelFactory
Expand Down
4 changes: 2 additions & 2 deletions src/interfaces/IERC5827.sol
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ interface IERC5827 is IERC20, IERC165 {
uint256 _recoveryRate
) external returns (bool success);

/// Overridden EIP-20 functions

/// @notice Returns approved max amount and recovery rate.
/// @return amount initial and maximum allowance given to spender
/// @return recoveryRate recovery amount per second
Expand All @@ -49,6 +47,8 @@ interface IERC5827 is IERC20, IERC165 {
view
returns (uint256 amount, uint256 recoveryRate);

/// Overridden EIP-20 functions

/// @notice Grants a (non-increasing) allowance of _value to _spender.
/// MUST clear set _recoveryRate to 0 on the corresponding renewable allowance, if any.
/// @param _spender allowed spender of token
Expand Down
5 changes: 5 additions & 0 deletions src/interfaces/IFunnel.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ import { IERC5827Proxy } from "./IERC5827Proxy.sol";
/// @title Interface for Funnel contracts for ERC20
/// @author Zac (zlace0x), zhongfu (zhongfu), Edison (edison0xyz)
interface IFunnel is IERC5827, IERC5827Proxy, IERC5827Payable {
/// @notice Called when the contract is being initialised.
/// @param _token contract address of the underlying ERC20 token
/// @dev Sets the INITIAL_CHAIN_ID and INITIAL_DOMAIN_SEPARATOR that might be used in future permit calls
function initialize(address _token) external;

/// @dev Invalid selector returned
error InvalidReturnSelector();

Expand Down
8 changes: 6 additions & 2 deletions src/lib/NativeMetaTransaction.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ abstract contract NativeMetaTransaction is EIP712, Nonces {
/// @param userAddress Address of the user that sent the meta-transaction
/// @param relayerAddress Address of the relayer that executed the meta-transaction
/// @param functionSignature Signature of the function
event MetaTransactionExecuted(address indexed userAddress, address payable relayerAddress, bytes functionSignature);
event MetaTransactionExecuted(
address indexed userAddress,
address payable indexed relayerAddress,
bytes functionSignature
);

/// @dev Function call is not successful
error FunctionCallError();
Expand Down Expand Up @@ -49,7 +53,7 @@ abstract contract NativeMetaTransaction is EIP712, Nonces {
bytes32 sigR,
bytes32 sigS,
uint8 sigV
) external payable returns (bytes memory data) {
) external returns (bytes memory data) {
MetaTransaction memory metaTx = MetaTransaction({
nonce: _nonces[userAddress]++,
from: userAddress,
Expand Down