UMAprotocol / protocol

UMA Protocol Running on Ethereum
https://uma.xyz
GNU Affero General Public License v3.0
361 stars 175 forks source link

feat(monitor-v2): validate osnap explanation #4620

Closed Reinis-FRP closed 11 months ago

Reinis-FRP commented 11 months ago

Motivation

oSnap bot replay protection does not fully protect against replay attacks where the attacker submits second proposal with changed casing for CIDv1 in explanation. The bot would think its a different Snapshot proposal while the IPFS gateway would still resolve the proposal with alternative casing.

Summary

Adds IPFS hash validation to oSnap verification logic.

Details

Uses multiformats library trying to parse explanation as CID. This parses only lower case CIDv1 strings, so any attack trying to pass upper casing would be detected.

Testing

Check a box to describe how you tested these changes and list the steps for reviewers to test.

Writing unit tests would require implementing mocked servers for ipfs content and serving graphql queries. Instead, this was tested as in production on Goerli:

2023-08-08 18:23:09 [info]: {
  "at": "OptimisticGovernorMonitor",
  "message": "Verified Transactions Proposed 📝",
  "mrkdwn": "<https://goerli.etherscan.io/address/0xB8034521BB1a343D556e5005680B3F17FFc74BeD|0xB80...c74BeD> made a proposal with hash 0xb600b7245b9097c9f1dd6cce1a8c1f12c50728380ddfd226879844fcb48d5db0 and assertion ID 0x946587fa0d50ccfaa451f9289a4f7b0a9585c7fec8d1e391472daa81c5a2fbbb on Optimistic Governor <https://goerli.etherscan.io/address/0xf2E0ed03c11B7A09afA1Ca8ECd376D13A1cb6b20|0xf2E...cb6b20> at Tue, 08 Aug 2023 18:22:48 GMT in transaction <https://goerli.etherscan.io/tx/0x8866feab2b1a5d343f21923450333e9c1a659867ab9b096a6b4f6bf9e0eb5074|0x886...eb5074>. Explanation: bafkreidfnv4irbs6n3lsjhpkbl54qe6gloxuqisdtxdwememtvoz4ns3qq. The proposal can be disputed till Tue, 08 Aug 2023 18:27:48 GMT: <https://testnet.oracle.uma.xyz/?transactionHash=0x8866feab2b1a5d343f21923450333e9c1a659867ab9b096a6b4f6bf9e0eb5074&eventIndex=200|View in UI>. <https://dashboard.tenderly.co/public/Uma/uma/simulator/29172c7f-49c6-4b9d-adc0-cc12d21cc3c3|Tenderly simulation successful>.",
  "rules": "I assert that this transaction proposal is valid according to the following rules: Proposals approved on Snapshot, as verified at https://snapshot.org/#/outcome.eth, are valid as long as there is a minimum quorum of 0 and a minimum voting period of 0 hours and it does not appear that the Snapshot voting system is being exploited or is otherwise unavailable. The quorum and voting period are minimum requirements for a proposal to be valid. Quorum and voting period values set for a specific proposal in Snapshot should be used if they are more strict than the rules parameter. The explanation included with the on-chain proposal must be the unique IPFS identifier for the specific Snapshot proposal that was approved or a unique identifier for a proposal in an alternative voting system approved by DAO social consensus if Snapshot is being exploited or is otherwise unavailable.",
  "notificationPath": "optimistic-governor",
  "bot-identifier": "optimistic-governor"
}
2023-08-08 18:33:11 [error]: {
  "at": "OptimisticGovernorMonitor",
  "message": "Unverified Transactions Proposed 🚩",
  "mrkdwn": "<https://goerli.etherscan.io/address/0xB8034521BB1a343D556e5005680B3F17FFc74BeD|0xB80...c74BeD> made a proposal with hash 0xb600b7245b9097c9f1dd6cce1a8c1f12c50728380ddfd226879844fcb48d5db0 and assertion ID 0x6a952de21e1ca18d283704890629b8bc89d8fc8577b4507b033da4fa35754d4c on Optimistic Governor <https://goerli.etherscan.io/address/0xf2E0ed03c11B7A09afA1Ca8ECd376D13A1cb6b20|0xf2E...cb6b20> at Tue, 08 Aug 2023 18:33:00 GMT in transaction <https://goerli.etherscan.io/tx/0x74173cdf695302a9bb164dfb766cd21daf52361686a00154db9c55492eca5de3|0x741...ca5de3>. Explanation: BAFKREIDFNV4IRBS6N3LSJHPKBL54QE6GLOXUQISDTXDWEMEMTVOZ4NS3QQ. The proposal can be disputed till Tue, 08 Aug 2023 18:38:00 GMT: <https://testnet.oracle.uma.xyz/?transactionHash=0x74173cdf695302a9bb164dfb766cd21daf52361686a00154db9c55492eca5de3&eventIndex=12|View in UI>. <https://dashboard.tenderly.co/public/Uma/uma/simulator/8a9e60d2-96fd-464f-a0a6-e73fca7985a1|Tenderly simulation successful>.",
  "rules": "I assert that this transaction proposal is valid according to the following rules: Proposals approved on Snapshot, as verified at https://snapshot.org/#/outcome.eth, are valid as long as there is a minimum quorum of 0 and a minimum voting period of 0 hours and it does not appear that the Snapshot voting system is being exploited or is otherwise unavailable. The quorum and voting period are minimum requirements for a proposal to be valid. Quorum and voting period values set for a specific proposal in Snapshot should be used if they are more strict than the rules parameter. The explanation included with the on-chain proposal must be the unique IPFS identifier for the specific Snapshot proposal that was approved or a unique identifier for a proposal in an alternative voting system approved by DAO social consensus if Snapshot is being exploited or is otherwise unavailable.",
  "notificationPath": "optimistic-governor",
  "verificationError": "IPFS hash BAFKREIDFNV4IRBS6N3LSJHPKBL54QE6GLOXUQISDTXDWEMEMTVOZ4NS3QQ is not valid",
  "bot-identifier": "optimistic-governor"
}

Issue(s)

Fixes https://linear.app/uma/issue/UMA-1640/osnap-bot-should-handle-ipfs-cid-casing

linear[bot] commented 11 months ago
UMA-1640 oSnap bot should handle IPFS CID casing

Even after implementing [UMA-1608](https://linear.app/uma/issue/UMA-1608/bug-verification-bot-does-not-defend-against-replay-attacks) , this does not fully protect against replay attacks where the attacker submits second proposal with changed casing for CIDv1 in explanation. The bot would think its a different Snapshot proposal while the IPFS gateway would still resolve the proposal with alternative casing. We should fix this by performing additional encoding/decoding of explanation to detect CID version and allow different casing when its CIDv1

Reinis-FRP commented 11 months ago

This changes look good to me!

Personally I think having tests in this project makes a lot of sense and worth the exploration! Could this be used to mock ipfs? https://github.com/httptoolkit/mockipfs

Agree, it would be worth adding tests, but the challenging part is we need to mock both IPFS and GraphQL responses. mockipfs might be useful if we did access IPFS nodes directly, but here we use gateway provider and access contents with a simple fetch (without ipfs client)