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 #49

Merged
merged 7 commits into from
Dec 8, 2022
Merged

Fix/audits #49

merged 7 commits into from
Dec 8, 2022

Conversation

edison0xyz
Copy link
Contributor

Fixed 8 issues in audits. Exact issue is referenced in commit messages.

@edison0xyz edison0xyz requested review from zhongfu and zlace0x December 1, 2022 09:36
Copy link
Contributor

@zhongfu zhongfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for #36 (floating pragma):

do we want to lock all contracts to 0.8.17, or is 0.8.15+ fine for some of them?

src/lib/EIP712.sol:pragma solidity ^0.8.15;
src/lib/MetaTxContext.sol:pragma solidity ^0.8.15;
src/lib/NativeMetaTransaction.sol:pragma solidity ^0.8.15;
src/lib/Nonces.sol:pragma solidity ^0.8.15;
test/TestSetup.sol:pragma solidity ^0.8.15;
src/FunnelFactory.sol:pragma solidity 0.8.17;
src/Funnel.sol:pragma solidity 0.8.17;
src/interfaces/IERC5827Payable.sol:pragma solidity 0.8.17;
src/interfaces/IERC5827Proxy.sol:pragma solidity 0.8.17;
src/interfaces/IERC5827.sol:pragma solidity 0.8.17;
src/interfaces/IERC5827Spender.sol:pragma solidity 0.8.17;
src/interfaces/IFunnelFactory.sol:pragma solidity 0.8.17;
src/interfaces/IFunnel.sol:pragma solidity 0.8.17;
src/mocks/MockERC1271.sol:pragma solidity 0.8.17;
src/mocks/MockSpenderReceiver.sol:pragma solidity 0.8.17;
src/mocks/TestERC20Token.sol:pragma solidity 0.8.17;
test/ERC20TestSuite.sol:pragma solidity 0.8.17;
test/ERC5827TestSuite.sol:pragma solidity 0.8.17;
test/FunnelERC20.t.sol:pragma solidity 0.8.17;
test/FunnelFactory.t.sol:pragma solidity 0.8.17;
test/Funnel.t.sol:pragma solidity 0.8.17;
test/MockSpenderReceiver.t.sol:pragma solidity 0.8.17;

for #45 (functions that can be declared external):

  • Funnel.allowance() can probably have external visibility too -- internal functions can (and already) use _remainingAllowance() for that

for #44 (unclear use of virtual specifier):

there's a few functions that still have virtual specifiers that we can possibly omit:

  • Funnel.computeDomainSeparator(): not much of a reason for anything in there to change in inheriting contracts (e.g. we already use name() for name)
  • Funnel.permit(), Funnel.permitRenewable()
  • Funnel._checkOnTransferReceived(), Funnel._checkOnApprovalReceived()
  • Funnel._fallback(): though we may want to override this in inheriting contracts (e.g. to limit which functions we pass through to the base token contract???)

on the other hand:

  • Funnel.supportsInterface(): may want to be able to override this in inheriting contracts, e.g. to indicate that the inheriting contract also supports other interfaces

with that said, we could probably just toss in the virtual specifier if we really do need them in the future

src/Funnel.sol Outdated Show resolved Hide resolved
src/lib/NativeMetaTransaction.sol Show resolved Hide resolved
- reorder imports, fix qoutes
- removed virtual for _checkOn* functions
- added virtual to supportsInterface
- fix compiler version to 0.8.17
@zlace0x zlace0x merged commit 8853294 into main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment