Find file History
Pull request Compare This branch is 38 commits ahead, 13 commits behind ethereum-alarm-clock:master.
Fetching latest commit…
Cannot retrieve the latest commit at this time.
Permalink
Type Name Latest commit message Commit time
..
Failed to load latest commit information.
code-review
test
README.md

README.md

Ethereum Alarm Clock Smart Contract Audit

Summary

The Ethereum Alarm Clock (EAC) was originally created by Piper Merriam in 2015. Chronologic have been working with Piper Merriam to enhance EAC.

Bok Consulting Pty Ltd was commissioned to perform an audit on the Ethereum smart contracts built for the Ethereum Alarm Clock.

This audit has been conducted on the EAC source code in commits 252a7a9, c3f26bc, 3685c4f, a7c70b4 and 1da981c.

The user interface for testing the EAC smart contracts on the Kovan testnet is at http://chronologic-dev.s3-website-us-east-1.amazonaws.com/.

The documentation for the EAC is at https://ethereum-alarm-clock.readthedocs.io/en/latest/index.html.

No potential vulnerabilities have been identified in the EAC smart contracts.

There is one outstanding LOW IMPORTANCE issue to resolve to improve the RecurringPayment example and Chronologic have responded that this will be completed in the near future.

There are also two outstanding LOW IMPORTANCE issues to resolve to improve the DelayedPayment example.



Table Of Contents



Recommendations

  • LOW IMPORTANCE scheduler/BlockScheduler.sol and scheduler/TimestampScheduler.sol both have a comment // Sets the factoryAddress variable found in SchedulerInterface contract. but factoryAddress is defined in scheduler/BaseScheduler.sol and not in the Interface/SchedulerInterface. These comments should be updated
  • LOW IMPORTANCE The constructors for RequestFactory.sol, Scheduler/BlockScheduler.sol, Scheduler/TimestampScheduler.sol and _examples/DelayedPayment.sol should be updated to use the constructor(...) keyword introduced in Solidity 0.4.21, if any source code is updated
  • LOW IMPORTANCE RequestLib.getEXECUTION_GAS_OVERHEAD() should be using the pure modifier instead of the view modifier, if any source code is updated
  • LOW IMPORTANCE Events in libraries are not automatically included in the ABI for contracts that call the library. The current workaround is to duplicate the events in the contracts that call the library. One reference. In EAC for example, RequestLib's Aborted(...), Cancelled(...), Claimed() and Executed(...) events are not available in the ABI for TransactionRequestCore - test/TransactionRequestCore.js#L46
  • LOW IMPORTANCE The comments for PaymentLib.validateEndowment(...) referring to maxMultiplier may be out of date
  • LOW IMPORTANCE ClaimLib.claim(...) has a bool return status that is not set, and is not used in RequestLib.claim(...)
  • LOW IMPORTANCE The comment for RequestScheduleLib.isBeforeClaimWindow(...) refers to freeze period but should refer to claim period
  • LOW IMPORTANCE SafeMath is not used in ClaimLib
  • LOW IMPORTANCE The index number for uintArgs[7] should be swapped with uintArgs[6] in the comment above TransactionRequestCore.initialize(...)
  • MEDIUM IMPORTANCE Please review the issue below on a residual amount remaining in the DelayedPayment contract
  • NOTE Note that if there is a input parameter validation error, the ValidationError(...) events from RequestFactory will never get generated because BaseScheduler.schedule(...) will throw an error if the validation fails, and the event logs will not be persisted on the blockchain
  • LOW IMPORTANCE The RecurringPayment example contract will need to be updated to remove the same residual amount issue as already resolved in the DelayedPayment contract
    • Chronologic have responded that this will be completed in the near future
  • LOW IMPORTANCE DelayedPayment.payout() - If the ETH deposited in this contract >= 2 x value, the integer multiple excess over the value can be sent to the recipient, and this can be called by anyone once the lockedUntil time is passed. Normally the user scheduling the payment should not send much more than the required scheduled payment.
  • LOW IMPORTANCE DelayedPayment.collectRemaining() - This can be called by anyone anytime and will send the ETH back to the owner, causing the future scheduled payments to fail


Issues

Residual Amount Remaining In The DelayedPayment Contract

In my testing script (and the corresponding results, I have deployed the EAC contracts and used the DelayedPayment.sol example with the following parameters:

  • The Schedule Creator account deploys DelayedPayment.sol with numBlocks = 20, sending 10 ETH with the payment scheduled to be sent to Payment Recipient
  • The Executor account claims the request, sending 0.1 ETH with the transaction
  • The Executor account waits the 20 blocks and executes the request

This results in the following table:

 # Account                                             EtherBalanceChange                 (Token A) WETH                  (Token B) DAI Name
-- ------------------------------------------ --------------------------- ------------------------------ ------------------------------ ---------------------------
 0 0xa00af22d07c87d96eeeb0ed583f8f6ac7812827e        0.017037202000000000           0.000000000000000000           0.000000000000000000 Account #0 - Miner
 1 0xa11aae29840fbb5c86e6fd4cf809eba183aef433       -0.011958348000000000           0.000000000000000000           0.000000000000000000 Account #1 - Contract Owner
 2 0xa22ab8a9d641ce77e06d98b7d7065d324d3d6976        0.000000020000000000           0.000000000000000000           0.000000000000000000 Account #2 - Fee Recipient
 3 0xa33a6c312d9ad0e0f2e95541beed0cc081621fd0      -10.001596498000000000           0.000000000000000000           0.000000000000000000 Account #3 - Schedule Creator
 4 0xa44a08d3f6933c69212114bb66e2df1813651844        9.900000000000000000           0.000000000000000000           0.000000000000000000 Account #4 - Payment Recipient
 5 0xa55a151eb00fded1634d27d1127b4be4627079ea        0.001063783200000000           0.000000000000000000           0.000000000000000000 Account #5 - Executor
...
12 0x8244333e424f27d9992f55be2ab362de4203ef61        0.000000000000000000           0.000000000000000000           0.000000000000000000 MathLib
13 0x60c8e00dafb889136ebd2dc558d0d00a69b7a84b        0.000000000000000000           0.000000000000000000           0.000000000000000000 PaymentLib
14 0x3529089fbacf865e6771ddb8a76ac997963a5393        0.000000000000000000           0.000000000000000000           0.000000000000000000 RequestScheduleLib
15 0xee0ec07598ed3d84fc4ff0ad4a6d70300f79d812        0.000000000000000000           0.000000000000000000           0.000000000000000000 IterTools
16 0xcba3446221eaad5f03a413e070be7978bcf5beb9        0.000000000000000000           0.000000000000000000           0.000000000000000000 RequestLib
17 0xe17585e1e925353038159ca920ff19f47207d0a0        0.000000000000000000           0.000000000000000000           0.000000000000000000 TransactionRequestCore
18 0x44f3bfeccc26c26313d1b5d4448a0b84e45a391b        0.000000000000000000           0.000000000000000000           0.000000000000000000 RequestFactory
19 0xcc8461036413b0fb03a632e408781b40dcd630b4        0.000000000000000000           0.000000000000000000           0.000000000000000000 BlockScheduler
20 0x9f1406f34716517d86b8ba237501a97c19699c39        0.000000000000000000           0.000000000000000000           0.000000000000000000 TimestampScheduler
21 0xb34c33eaf9d9c3c959f095edeb5a247634c98639        0.095453840800000000           0.000000000000000000           0.000000000000000000 DelayedPayment
22 0xa2aa7dfbfcba85660634acfcb39167a7304f7fad        0.000000000000000000           0.000000000000000000           0.000000000000000000 DelayedPaymentRequest
-- ------------------------------------------ --------------------------- ------------------------------ ------------------------------ ---------------------------
                                                                                    0.000000000000000000           0.000000000000000000 Total Token Balances
-- ------------------------------------------ --------------------------- ------------------------------ ------------------------------ ---------------------------

Note that there is a residual 0.095453840800000000 ETH balance in the DelayedPayment contract.

I've added additional debugging information to RequestLib to produce the following output::

txRequest.address=0xa2aa7dfbfcba85660634acfcb39167a7304f7fad
  claimData.claimedBy=0xa55a151eb00fded1634d27d1127b4be4627079ea
  meta.createdBy=0xcc8461036413b0fb03a632e408781b40dcd630b4
  meta.owner=0xb34c33eaf9d9c3c959f095edeb5a247634c98639
  paymentData.feeRecipient=0xa22ab8a9d641ce77e06d98b7d7065d324d3d6976
  paymentData.bountyBenefactor=0xa55a151eb00fded1634d27d1127b4be4627079ea
  txnData.toAddress=0xb34c33eaf9d9c3c959f095edeb5a247634c98639
  meta.isCancelled=false
  meta.wasCalled=true
  meta.wasSuccessful=true
  claimData.claimDeposit=0.000000000000000000 0
  paymentData.fee=0.000000020000000000 20000000000
  paymentData.feeOwed=0.000000000000000000 0
  paymentData.bounty=0.000000020000000000 20000000000
  paymentData.bountyOwed=0.000000000000000000 0
  schedule.claimWindowSize=255
  (schedule.firstClaimBlock=18560 = freezeStart-claimWindowSize)
  schedule.freezePeriod=10
  (schedule.freezeStart=18815) = windowStart - freezePeriod
  schedule.reservedWindowSize=16
  (schedule.reservedWindowEnd=18841) = windowStart + reserveWindowSize
  schedule.temporalUnit=1
  schedule.windowSize=255
  schedule.windowStart=18825
  (schedule.windowEnd=19080) = windowStart + windowSize
  txnData.callGas=200000
  txnData.callValue=0
  txnData.gasPrice=0.000000020000000000 20000000000
  claimData.requiredDeposit=0.000000030000000000 30000000000
  claimData.paymentModifier=96
  txnData.callData=0x
txRequest.Executed 0 #18829 {"bounty":"104546139200000000","fee":"20000000000","measuredGasConsumption":"227306"}
txRequest.LogUint 0 #18829 source=execute text=Before sendTransaction - request.balance value=200000000000000000 0.2
txRequest.LogUint 1 #18829 source=execute text=Before sendTransaction - self.txnData.toAddress.balance value=9900000000000000000 9.9
txRequest.LogUint 2 #18829 source=execute text=After sendTransaction - request.balance value=200000000000000000 0.2
txRequest.LogUint 3 #18829 source=execute text=After sendTransaction - self.txnData.toAddress.balance value=0 0
txRequest.LogUint 4 #18829 source=execute text=feeOwed before value=0 0
txRequest.LogUint 5 #18829 source=execute text=getFee() value=20000000000 2e-8
txRequest.LogUint 6 #18829 source=execute text=feeOwed after value=20000000000 2e-8
txRequest.LogUint 7 #18829 source=execute text=bountyOwed before value=100000019200000000 0.1000000192
txRequest.LogUint 8 #18829 source=execute text=bountyOwed after value=104546139200000000 0.1045461392
txRequest.LogUint 9 #18829 source=execute text=Before sendBounty - request.balance value=199999980000000000 0.19999998
txRequest.LogUint 10 #18829 source=execute text=Before sendBounty - self.txnData.toAddress.balance value=0 0
txRequest.LogUint 11 #18829 source=execute text=After sendBounty - request.balance value=95453840800000000 0.0954538408
txRequest.LogUint 12 #18829 source=execute text=After sendBounty - self.txnData.toAddress.balance value=0 0
txRequest.LogUint 13 #18829 source=execute text=After _sendOwnerEther - self.txnData.toAddress.balance value=95453840800000000 0.0954538408

This 0.095453840800000000 ETH amount comes from RequestLib.execute(...) section:

        // Attempt to send the bounty. as with `.sendFee()` it may fail and need to be caled after execution.
        emit LogUint("execute", "Before sendBounty - request.balance", address(this).balance);
        emit LogUint("execute", "Before sendBounty - self.txnData.toAddress.balance", self.txnData.toAddress.balance);
        self.paymentData.sendBounty();
        emit LogUint("execute", "After sendBounty - request.balance", address(this).balance);
        emit LogUint("execute", "After sendBounty - self.txnData.toAddress.balance", self.txnData.toAddress.balance);

This residual can only be sent to the Payment Recipient by executing DelayedPayment.payout() after the execution period.

Resolution:

ChronoLogic comment

The residual value on the DelayedPayment contract is the remaining value send back by the scheduler afer execution. To improve the UX we've modified the example to:

  • use computeEndowment function to estimate the required amount of ETH - note that we are assuming 200k gas for execution, while any lower actual value will ends up with remaining ETH on the scheduled tx contract which then is send back again to DelayedPayment
  • added collectRemaining to transfer back the remaining amount of ETH to owner
  • added value to indicate the exact amount of ETH to be sent to the receipient

All changes available in PR https://github.com/ethereum-alarm-clock/ethereum-alarm-clock/pull/148



Potential Vulnerabilities

No potential vulnerabilities have been identified in the EAC smart contracts.



Scope

This audit is into the technical aspects of the EAC smart contracts. The primary aim of this audit is to ensure that funds stored in these contracts are not easily attacked or stolen by third parties. The secondary aim of this audit is to ensure the coded algorithms work as expected. This audit does not guarantee that that the code is bugfree, but intends to highlight any areas of weaknesses.



Risks

None identified in the EAC framework. Users of EAC are able to deploy their own payment smart contracts, and these should be validated independently for contracts holding larger amounts of ETH and/or tokens.



Testing

Details of the testing environment can be found in test.

The following functions were tested using the script test/01_test1.sh with the summary results saved in test/test1results.txt and the detailed output saved in test/test1output.txt:

  • Deploy Libraries #1
    • Deploy MathLib
    • Deploy PaymentLib
    • Deploy RequestScheduleLib
    • Deploy IterTools
  • Deploy Libraries #2
    • Deploy RequestLib
    • Deploy TransactionRequestCore
  • Deploy RequestFactory
  • Deploy Schedulers
    • Deploy BlockScheduler
    • Deploy TimestampScheduler
  • Execute Delayed Payment
    • Schedule Delayed Payment
    • Claim Delayed Payment
    • Execute Delayed Payment


Notes

Periods

The following fields are from the DelayedPayment testing results. Items in brackets are calculated on the fly. Items are rearranged into their order:

schedule.claimWindowSize=255
schedule.freezePeriod=10
schedule.reservedWindowSize=16
schedule.windowSize=255
schedule.temporalUnit=1

(schedule.firstClaimBlock=99 = freezeStart-claimWindowSize)
(schedule.freezeStart=354) = windowStart - freezePeriod
schedule.windowStart=364
(schedule.reservedWindowEnd=380) = windowStart + reserveWindowSize
(schedule.windowEnd=619) = windowStart + windowSize


Code Review

Exit Points For Ethers

Check exit points for ethers:

Core

  • RequestFactory.sol: msg.sender.transfer(msg.value);
  • Library/ClaimLib.sol: return self.claimedBy.send(depositAmount);
  • Library/PaymentLib.sol: return self.feeRecipient.send(feeAmount);
  • Library/PaymentLib.sol: return self.bountyBenefactor.send(bountyAmount);
  • Library/RequestLib.sol: rewardBenefactor.transfer(rewardOwed);
  • Library/RequestLib.sol: return recipient.send(ownerRefund);

Examples

  • _examples/DelayedPayment.sol: recipient.transfer(address(this).balance);
  • _examples/RecurringPayment.sol: recipient.transfer(paymentValue); (to be updated)

contract


contract/_examples


contract/Interface


contract/Library


contract/Scheduler


contract/zeppelin


Excluded - Only Used For Testing


Compiler Warnings

The first and the third compiler warnings are due to the new constructor(...) keyword available from Solidity 0.4.21 onwards.

RequestFactory.sol:22:5: Warning: Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
    function RequestFactory(
    ^ (Relevant source part starts here and spans across multiple lines).
Library/RequestLib.sol:412:5: Warning: Function state mutability can be restricted to pure
    function getEXECUTION_GAS_OVERHEAD()
    ^ (Relevant source part starts here and spans across multiple lines).
./_examples/DelayedPayment.sol:14:5: Warning: Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
    function DelayedPayment(
    ^ (Relevant source part starts here and spans across multiple lines).


(c) BokkyPooBah / Bok Consulting Pty Ltd for Ethereum Alarm Clock and Chronologic - Aug 27 2018. The MIT Licence.