search

UMAprotocol / protocol

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

Bug by typo in polymarket notifier #4614

Closed MarcoBeyer closed 4 months ago

MarcoBeyer commented 11 months ago

https://github.com/UMAprotocol/protocol/blob/38ac297998f3df653a5bb0e330917a8cd7c712ed/packages/polymarket-notifier/src/polymarketNotifier.js#L196-L197

polymarketContract.resolveBy === binaryAdapterAddress ? binaryAdapterContract : ctfAdapterContract;

Is this a typo and should be resolvedBy instead of resolveBy? This bug will always return the ctfAdapterContract because undefined !== binaryAdapterAddress.

@md0x Should be your field of expertise

MarcoBeyer commented 11 months ago

By fixing this another error will occur Both contracts have the same function signature so aggregateTransactionsAndCall will always call _decodeOutput.

https://github.com/UMAprotocol/protocol/blob/38ac297998f3df653a5bb0e330917a8cd7c712ed/packages/financial-templates-lib/src/helpers/multicall.ts#L11-L13 Will return the latest added function, because both ABIs have a question function with the same inputs. So they have the same function id/signature.

In the case of the polymarket notifier https://github.com/UMAprotocol/protocol/blob/38ac297998f3df653a5bb0e330917a8cd7c712ed/packages/polymarket-notifier/src/polymarketNotifier.js#L34-L36

So it will try to decode with the wrong ABI which triggers a Overflow exception.

mrice32 commented 6 months ago

Sorry, we're so slow in seeing this. I've reached out to @md0x to see if he can take a look!

md0x commented 4 months ago

@MarcoBeyer , thank you for pointing out those issues in our project! We apologize for the delay in responding to your findings.

Regarding the code in polymarketNotifier.js, you're absolutely right about the typo. We had already transitioned to using a new version of the polymarket notifier, which you can find here: MonitorProposalsOrderBook.ts.

More crucially, your observation regarding the usage of abiDecoder was spot-on. We've taken steps to address this issue in our new adapter, as shown in this update: common.ts.

We had implemented various try-catch blocks to mitigate errors from multicall decoding, which could result in missing markets in the notifier. However, your insights were indeed valuable and have contributed significantly to enhancing our project.

Thank you again for your contribution and attention to detail. Your input has been helpful in improving our project.