search

OpenST / mosaic-contracts

Mosaic-0: Gateways and anchors on top of Ethereum to scale DApps
https://discuss.openst.org/c/mosaic
MIT License
114 stars 31 forks source link

Changing state before transfering base token #620

Closed 0xsarvesh closed 5 years ago

0xsarvesh commented 5 years ago

Fixes #605

State change operations are done before token transfers in order to enhance defense against reentrancy attack.

⚠️ Slither still identifies some vulnerability even after fixing them. It may be the default behavior of slither or there is something which needs to be fixed. In both scenarios, it's out of the scope of this ticket. Any input on this is appreciated. :pray::skin-tone-3:

EIP20CoGateway.progressRevertRedeem (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#641-715) sends eth to arbirary user
    Dangerous calls:
    - burner.transfer(bounty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#703)
    - burner.transfer(penalty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#706)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations
INFO:Detectors:
EIP20CoGateway.progressRedeemInternal (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#1116-1150) sends eth to arbirary user
    Dangerous calls:
    - msg.sender.transfer(stakedBounty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#1139)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations
schemar commented 5 years ago

Fixes #605

State change operations are done before token transfers in order to enhance defense against reentrancy attack.

⚠️ Slither still identifies some vulnerability even after fixing them. It may be the default behavior of slither or there is something which needs to be fixed. In both scenarios, it's out of the scope of this ticket. Any input on this is appreciated. 🙏:skin-tone-3:

EIP20CoGateway.progressRevertRedeem (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#641-715) sends eth to arbirary user
  Dangerous calls:
  - burner.transfer(bounty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#703)
  - burner.transfer(penalty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#706)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations
INFO:Detectors:
EIP20CoGateway.progressRedeemInternal (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#1116-1150) sends eth to arbirary user
  Dangerous calls:
  - msg.sender.transfer(stakedBounty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#1139)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations

Regarding the first one: I would say it is a false-positive, as the burner address is not arbitrary. It is set at construction and cannot be changed.

Regarding the second one: I would say that is a false-positive as well. The function progressRedeemInternal is only called from progressRedeem and progressRedeemWithProof. For progressRedeem the secret must be known. For progressRedeemWithProof it is correct that it can be called by an arbitrary address. However, strictly speaking we do have a front-running issue with revealing the secret ...

Anyways, when slither reports something it means that it must be checked by a human, because slither doesn't know for sure. In these cases, for example, confidence was "medium".

0xsarvesh commented 5 years ago

Fixes #605 State change operations are done before token transfers in order to enhance defense against reentrancy attack. ⚠️ Slither still identifies some vulnerability even after fixing them. It may be the default behavior of slither or there is something which needs to be fixed. In both scenarios, it's out of the scope of this ticket. Any input on this is appreciated. 🙏:skin-tone-3:

EIP20CoGateway.progressRevertRedeem (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#641-715) sends eth to arbirary user
    Dangerous calls:
    - burner.transfer(bounty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#703)
    - burner.transfer(penalty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#706)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations
INFO:Detectors:
EIP20CoGateway.progressRedeemInternal (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#1116-1150) sends eth to arbirary user
    Dangerous calls:
    - msg.sender.transfer(stakedBounty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#1139)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations

Regarding the first one: I would say it is a false-positive, as the burner address is not arbitrary. It is set at construction and cannot be changed.

Regarding the second one: I would say that is a false-positive as well. The function progressRedeemInternal is only called from progressRedeem and progressRedeemWithProof. For progressRedeem the secret must be known. For progressRedeemWithProof it is correct that it can be called by an arbitrary address. However, strictly speaking we do have a front-running issue with revealing the secret ...

Anyways, when slither reports something it means that it must be checked by a human, because slither doesn't know for sure. In these cases, for example, confidence was "medium".

Thanks for the review and detailed feedback. 🙌

As you have observed, Message bus state machine will always protect against reentrancy. This deletion is like another safety measure following the general practice of changing state before token transfer.

Do you think there should be require to ensure mapping is not already deleted? Happy to discuss more.

schemar commented 5 years ago

Fixes #605 State change operations are done before token transfers in order to enhance defense against reentrancy attack. ⚠️ Slither still identifies some vulnerability even after fixing them. It may be the default behavior of slither or there is something which needs to be fixed. In both scenarios, it's out of the scope of this ticket. Any input on this is appreciated. 🙏:skin-tone-3:

EIP20CoGateway.progressRevertRedeem (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#641-715) sends eth to arbirary user
  Dangerous calls:
  - burner.transfer(bounty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#703)
  - burner.transfer(penalty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#706)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations
INFO:Detectors:
EIP20CoGateway.progressRedeemInternal (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#1116-1150) sends eth to arbirary user
  Dangerous calls:
  - msg.sender.transfer(stakedBounty) (mosaic-contracts/contracts/gateway/EIP20CoGateway.sol#1139)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations

Regarding the first one: I would say it is a false-positive, as the burner address is not arbitrary. It is set at construction and cannot be changed. Regarding the second one: I would say that is a false-positive as well. The function progressRedeemInternal is only called from progressRedeem and progressRedeemWithProof. For progressRedeem the secret must be known. For progressRedeemWithProof it is correct that it can be called by an arbitrary address. However, strictly speaking we do have a front-running issue with revealing the secret ... Anyways, when slither reports something it means that it must be checked by a human, because slither doesn't know for sure. In these cases, for example, confidence was "medium".

Thanks for the review and detailed feedback. 🙌

As you have observed, Message bus state machine will always protect against reentrancy. This deletion is like another safety measure following the general practice of changing state before token transfer.

Do you think there should be require to ensure mapping is not already deleted? Happy to discuss more.

Ah yes. You are right. I only focused on the mappings and didn't regard the state machine. Let me resolve what I can 😊

schemar commented 5 years ago

OK now I see. It was "invisible" to me due to it being handled in the message bus and it relies on calling the right methods on the message bus in the right order. Overall the Gateway/MessageBus architecture is very opaque and probably also not so easy to understand by the people that would do a security assessment. @sarvesh-ost should we add to the @dev section of the affected functions that correct order of execution is handled by the message bus state machine? What do you think?

0xsarvesh commented 5 years ago

Looks good 🐨

Only beneficiary/beneficiary_. I think one is sufficient. And the question whether to add comments about the state machine.

@schemar Do you still want me to add dev comment about execution sequence handled by message bus, as we have explicit checks to verify request exists(it's no more dependent on message bus)

If yes, Do you have a suggestion?