search

Joystream / argo-bridge

Cross-chain token bridge between Joystream and EVM blockchains
https://argo-bridge-app.vercel.app
ISC License
1 stars 1 forks source link

Slither: static code analysis #2

Open mnaamani opened 1 month ago

mnaamani commented 1 month ago

I ran slither against the smart contracts. Not all issues are serious, but worth looking over. Perhaps one we can look into is regards to the version of solidity compiler that is being selected. We are targeting ^0.8.24; but the versions of the openzepplin contracts maybe be slightly older and is forcing the build to use v0.8.20.

Version constraint ^0.8.20 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html)
    - VerbatimInvalidDeduplication
    - FullInlinerNonExpressionSplitArgumentEvaluationOrder
    - MissingSideEffectsOnSelectorAccess.

Full output from analysis:

Red

INFO:Detectors:
ArgoBridgeV1.withdrawBridgeFees() (contracts/ArgoBridgeV1.sol#299-303) sends eth to arbitrary user
    Dangerous calls:
    - address(msg.sender).transfer(balance) (contracts/ArgoBridgeV1.sol#301)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#functions-that-send-ether-to-arbitrary-destinations
INFO:Detectors:
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) has bitwise-xor operator ^ instead of the exponentiation operator **: 
     - inverse = (3 * denominator) ^ 2 (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#184)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-exponentiation

Yellow

INFO:Detectors:
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
    - denominator = denominator / twos (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#169)
    - inverse = (3 * denominator) ^ 2 (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#184)
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
    - denominator = denominator / twos (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#169)
    - inverse *= 2 - denominator * inverse (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#188)
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
    - denominator = denominator / twos (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#169)
    - inverse *= 2 - denominator * inverse (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#189)
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
    - denominator = denominator / twos (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#169)
    - inverse *= 2 - denominator * inverse (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#190)
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
    - denominator = denominator / twos (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#169)
    - inverse *= 2 - denominator * inverse (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#191)
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
    - denominator = denominator / twos (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#169)
    - inverse *= 2 - denominator * inverse (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#192)
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
    - denominator = denominator / twos (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#169)
    - inverse *= 2 - denominator * inverse (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#193)
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) performs a multiplication on the result of a division:
    - prod0 = prod0 / twos (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#172)
    - result = prod0 * inverse (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#199)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply

Green

INFO:Detectors:
ERC20Permit.constructor(string).name (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol#39) shadows:
    - ERC20.name() (../../node_modules/@openzeppelin/contracts/token/ERC20/ERC20.sol#58-60) (function)
    - IERC20Metadata.name() (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol#15) (function)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#local-variable-shadowing
INFO:Detectors:
Reentrancy in ArgoBridgeV1.requestTransferToJoystream(bytes32,uint256) (contracts/ArgoBridgeV1.sol#163-179):
    External calls:
    - joystreamErc20.burnFrom(msg.sender,amount) (contracts/ArgoBridgeV1.sol#174)
    State variables written after the call(s):
    - transferIdCounter += 1 (contracts/ArgoBridgeV1.sol#178)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-2
INFO:Detectors:
Reentrancy in ArgoBridgeV1.completeTransferToEth(uint256,address,uint256) (contracts/ArgoBridgeV1.sol#198-219):
    External calls:
    - joystreamErc20.mint(ethDestAddress,amount) (contracts/ArgoBridgeV1.sol#217)
    Event emitted after the call(s):
    - ArgoTransferToEthCompleted(joyTransferId,ethDestAddress,amount) (contracts/ArgoBridgeV1.sol#218)
Reentrancy in ArgoBridgeV1.requestTransferToJoystream(bytes32,uint256) (contracts/ArgoBridgeV1.sol#163-179):
    External calls:
    - joystreamErc20.burnFrom(msg.sender,amount) (contracts/ArgoBridgeV1.sol#174)
    Event emitted after the call(s):
    - ArgoTransferToJoystreamRequested(transferIdCounter,msg.sender,joyDestAccount,amount) (contracts/ArgoBridgeV1.sol#176)
Reentrancy in ArgoBridgeV1.revertTransferToJoystream(uint256,address,uint256,string) (contracts/ArgoBridgeV1.sol#227-249):
    External calls:
    - joystreamErc20.mint(revertAddress,revertAmount) (contracts/ArgoBridgeV1.sol#247)
    Event emitted after the call(s):
    - ArgoTransferToJoystreamReverted(ethTransferId,revertAddress,revertAmount,rationale) (contracts/ArgoBridgeV1.sol#248)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-3
INFO:Detectors:
ERC20Permit.permit(address,address,uint256,uint256,uint8,bytes32,bytes32) (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol#44-67) uses timestamp for comparisons
    Dangerous comparisons:
    - block.timestamp > deadline (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol#53)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp
INFO:Detectors:
ShortStrings.toString(ShortString) (../../node_modules/@openzeppelin/contracts/utils/ShortStrings.sol#63-73) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/ShortStrings.sol#68-71)
StorageSlot.getAddressSlot(bytes32) (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#59-64) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#61-63)
StorageSlot.getBooleanSlot(bytes32) (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#69-74) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#71-73)
StorageSlot.getBytes32Slot(bytes32) (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#79-84) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#81-83)
StorageSlot.getUint256Slot(bytes32) (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#89-94) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#91-93)
StorageSlot.getStringSlot(bytes32) (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#99-104) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#101-103)
StorageSlot.getStringSlot(string) (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#109-114) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#111-113)
StorageSlot.getBytesSlot(bytes32) (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#119-124) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#121-123)
StorageSlot.getBytesSlot(bytes) (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#129-134) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#131-133)
Strings.toString(uint256) (../../node_modules/@openzeppelin/contracts/utils/Strings.sol#24-44) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/Strings.sol#30-32)
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/Strings.sol#36-38)
ECDSA.tryRecover(bytes32,bytes) (../../node_modules/@openzeppelin/contracts/utils/cryptography/ECDSA.sol#56-73) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/cryptography/ECDSA.sol#64-68)
MessageHashUtils.toEthSignedMessageHash(bytes32) (../../node_modules/@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol#30-37) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol#32-36)
MessageHashUtils.toTypedDataHash(bytes32,bytes32) (../../node_modules/@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol#76-85) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol#78-84)
Math.mulDiv(uint256,uint256,uint256) (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#123-202) uses assembly
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#130-133)
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#154-161)
    - INLINE ASM (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#167-176)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#assembly-usage

INFO:Detectors:
2 different versions of Solidity are used:
    - Version constraint ^0.8.20 is used by:
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/access/AccessControl.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/access/IAccessControl.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/interfaces/IERC5267.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/interfaces/draft-IERC6093.sol#3)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/ERC20.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/IERC20.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/Context.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/Nonces.sol#3)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/ShortStrings.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#5)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/Strings.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/cryptography/ECDSA.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/cryptography/EIP712.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/introspection/ERC165.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/introspection/IERC165.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#4)
        -^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/math/SignedMath.sol#4)
    - Version constraint ^0.8.24 is used by:
        -^0.8.24 (contracts/ArgoBridgeV1.sol#2)
        -^0.8.24 (contracts/JoystreamERC20.sol#2)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used
INFO:Detectors:
Version constraint ^0.8.20 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html)
    - VerbatimInvalidDeduplication
    - FullInlinerNonExpressionSplitArgumentEvaluationOrder
    - MissingSideEffectsOnSelectorAccess.
It is used by:
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/access/AccessControl.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/access/IAccessControl.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/interfaces/IERC5267.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/interfaces/draft-IERC6093.sol#3)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/ERC20.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/IERC20.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/Context.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/Nonces.sol#3)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/ShortStrings.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/StorageSlot.sol#5)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/Strings.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/cryptography/ECDSA.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/cryptography/EIP712.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/introspection/ERC165.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/introspection/IERC165.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/math/Math.sol#4)
    - ^0.8.20 (../../node_modules/@openzeppelin/contracts/utils/math/SignedMath.sol#4)
Version constraint ^0.8.24 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html)
.
It is used by:
    - ^0.8.24 (contracts/ArgoBridgeV1.sol#2)
    - ^0.8.24 (contracts/JoystreamERC20.sol#2)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity
INFO:Detectors:
Function ERC20Permit.DOMAIN_SEPARATOR() (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol#80-82) is not in mixedCase
Function IERC20Permit.DOMAIN_SEPARATOR() (../../node_modules/@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol#89) is not in mixedCase
Function EIP712._EIP712Name() (../../node_modules/@openzeppelin/contracts/utils/cryptography/EIP712.sol#146-148) is not in mixedCase
Function EIP712._EIP712Version() (../../node_modules/@openzeppelin/contracts/utils/cryptography/EIP712.sol#157-159) is not in mixedCase
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions
INFO:Detectors:
Reentrancy in ArgoBridgeV1.withdrawBridgeFees() (contracts/ArgoBridgeV1.sol#299-303):
    External calls:
    - address(msg.sender).transfer(balance) (contracts/ArgoBridgeV1.sol#301)
    Event emitted after the call(s):
    - ArgoBridgeFeesWithdrawn(msg.sender,balance) (contracts/ArgoBridgeV1.sol#302)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-4
INFO:Detectors:
ShortStrings.slitherConstructorConstantVariables() (../../node_modules/@openzeppelin/contracts/utils/ShortStrings.sol#40-123) uses literals with too many digits:
    - FALLBACK_SENTINEL = 0x00000000000000000000000000000000000000000000000000000000000000FF (../../node_modules/@openzeppelin/contracts/utils/ShortStrings.sol#42)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#too-many-digits
INFO:Slither:. analyzed (26 contracts with 94 detectors), 39 result(s) found
kdembler commented 1 month ago

I have reviewed Solidity bugs fixed after 0.8.20 and none of them apply to our contracts.

Regarding the full output:

  1. Red: 1.1. ArgoBridgeV1.withdrawBridgeFees() implements access control so we're good 1.2. Math.mulDiv(uint256,uint256,uint256) is detected inside of openezeppelin math modules, I trust they know what they're doing
  2. Yellow: all the reported issues are also inside openezeppelin math, so not a problem.
  3. Green (ignoring issues reported in openezeppelin contracts): 3.1. joystreamErc20.burnFrom reentrancy - not applicable, standard ERC20 burnFrom does not do any external calls. Even if it did allow reentrancy, the only issue this would produce is potentially requesting 2 transfers with the same ID, which wouldn't be a huge issue. 3.2. joystreamErc20.mint reentrancy - like above, mint does not do any external calls. Even if it did, it would not impact the logic in any way. 3.3. Reentrancy in ArgoBridgeV1.withdrawBridgeFees() - not applicable, only an event is emitted after external call and this function is access controlled.

So all looks good to me, no actions to be taken