celo-org / celo-monorepo

Official repository for core projects comprising the Celo platform
https://celo.org
Apache License 2.0
684 stars 360 forks source link

[ContractKit] Boolean calls handled incorrectly on exception #3713

Closed keefertaylor closed 3 years ago

keefertaylor commented 4 years ago

When making a call to a contract wrapper which returns a boolean, ContractKit will return true if a solidity require() statement triggers a revert op code.

Consider the following program:

import { newKit } from '@celo/contractkit'

const nodeURL = 'https://rc1-forno.celo-testnet.org/'
const kit = newKit(nodeURL)

const downtimeSlasherContract = await kit.contracts.getDowntimeSlasher()

const isDown = await downtimeSlasherContract.isDown(99999999999, 0, 0)
console.log(`isDown: ${isDown}`)

This program outputs true. However, block 99999999999 doesn't exist on RC1 yet, so this require() is hit.

The response is

0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002c656e6420626c6f636b206d75737420626520736d616c6c6572207468616e2063757272656e7420626c6f636b0000000000000000000000000000000000000000

which is a revert op code. ContractKit is interpreting this revert code as a boolean and returning true.

Here's a complete debug log of the above program: https://gist.github.com/keefertaylor/a1dff437fb41703d06a9a903f5d9673a

Expected Behavior

Contract kit throws an exception.

Current Behavior

ContractKit returns true.

keefertaylor commented 4 years ago

Hey folks,

Happy to write a PR for this but I am having trouble tracing down where the parsing is actually done. If you have a code pointer it is possible I could fix this myself.

eelanagaraj commented 3 years ago

@keefertaylor I wasn't able to reproduce this (using a different function on Accounts.sol as the isDown function no longer exists for DowntimeSlasher). It looks like hitting a require within a Contract call is now raising an exception as expected. I'm going to go ahead and close this, but please reopen or open a new issue if you observe this with a different function! Thanks for flagging this in any case!