Skip to content

Instantly share code, notes, and snippets.

@nachomazzara

nachomazzara/rcn-oracles-audit.md

Last active October 31, 2020 18:00
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save nachomazzara/039d0de63ab4dba403085b7b642db69f to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save nachomazzara/039d0de63ab4dba403085b7b642db69f to your computer and use it in GitHub Desktop.
Download ZIP
RCN - Oracles - Security Review
Raw
rcn-oracles-audit.md

Sorted-Collection

SortedList.sol

High vulnerability

  • DoS when the list has more than ~5527 items. Every time the median is calculated, it loops through the entire list to get the size and then loop size / 2 to get the median itself. Consider storing and update the size of the list every time the user insert or remove a node along with the two nodes which are part of the median in the struct. The worst scenario for an insert could be n - 1 loop where n is the size of the list. Consider using a binary search in order to reduce as much as possible a possible DoS.

Medium vulnerability

  • If the size of the sorted list is 0, median will throw. I recommend to check the size and if it is equal to 0, returns 0.

Low vulnerability

  • Possible arithmetic overflow at median when trying to get the avg of middle values. For rates wouldn't be a problem, but if it is used as a library for other values, could be.

Notes

  • elements / 2 it is repeated three times at median function.

  • LEFT and RIGHT can just be named PREV and NEXT.

  • Dev notation of getAdjacent seems to be out of date. _direction is not an expected parameter.

  • Lines 59, 109, 130, 147, 179 are unused lines.

RCN-Oracle Audit

Pausable.sol

Notes

  • Paused and Started may have indexed the caller in order to allow traceability of who paused or started it.

MultiSourceOracle.sol

High vulnerability

  • If for some reason an Oracle is not being provided by a specific time and/or the network is congested, it will return outdated values. Consider adding an on-chain strategy to prevent readSample returns an outdated value. E.g: Check if, at n time passed, at least 51% of signers had provided a new value where n can be something in seconds configurable by each Oracle or global.

  • setUpgrade is not checking if the new implementation implements the standard Diaspore RCN Oracle interface 0xa265d8e0 found at RateOracle.sol. Oracles without the core methods described in the interface can prevent the systems to operate.

Medium vulnerability

  • setUpgrade should check if the address is different from address(this) in order to prevent infinite recursion.

Low vulnerability

  • If a contract is created on top of the oracle, variables as isymbol, iname, idecimals, itoken, icurrent could be useful. Consider changing the visibility type from private to internal .

  • Check if PausedProvider is a contract.

  • Function url is not used and based on the documentation could end up in a possible misunderstood. Consider removing it.

  • _rate can not be the maximum uint96. Consider making the check inclusive.

  • When a signer is removed, an event defined at OracleFactory.sol is emitted: RemoveSigner. This event is also emitted when the signer had been removed before. Emitting the event when actually no signer is removed could generate an inconsistency when tracing/indexing signer addition and removals because, at signer addition, the AddSigner event is not emitted if the signer had been added. Consider reverting with require(isSigner[_signer],, "signer not defined") instead of return.

Notes

  • name, symbol and decimals are part of the ERC20 standard. Receiving them as parameters could end up differing from the token implementation. Consider getting the value directly from the token contract if applicable because not all of them will be for ERC20.

  • Use address(0) for FIAT like USD or ARS (Argentinian peso) can be mixed with the invalid address. Consider using a specific address to represent coins outside the Ethereum Blockchain.

  • Methods can be forwarded to upgrade implementation if apply in order to reduce undesired calls.

  • Move up the check if the signer is already a signer or not to reduce gas cost at removeSigner.

  • nameOfSigner and providedBy are not being cleared when removing a signer.

  • readSample (external) do nothing with its parameter. Consider removing or using it.

  • Check if _equivalent is distinct to 0.

  • Consider using uint256 at providedBy to reduce the gas cost of a mask.

  • Consider check if providedBy[_signer] == _rate before removing and inserting the same value.

OracleFactory.sol

High vulnerability

  • If for some reason an oracle should be paused, all the oracles will be paused to. That will end up in pausing all the operations related to the oracles making an outage of the full system. Consider adding granular management to the paused functionality along with the global one.

  • Lack of ownership transfer and resilience. If an Oracle should be upgraded to a new implementation, a new Factory should be deployed too in order to use a new implementation to deploy the Oracles. Also, the maintenance of the two factories will be needed. Consider adding a way to transfer the ownership of the Oracles, and to deploy Oracles based on an implementation stored as a contract variable. Another solution could be to use a Proxy for the Oracles to change its implementation and sort out the upgrade functionality that they already have.

Low vulnerability

  • Events have no indexed parameters and they are all emitted by the factory. That would make really hard for tracking, indexing and debugging. Consider moving Oracle's related events as RemoveSigner, AddSigner, Provide to the implementation or add the indexed keyword to the parameters which allow filtering and traceability.

Notes

  • A re-entrancy ward could be added to methods that call the Oracles in order to prevent undesired behaviors and complex attack vectors.

  • provideMultiple receives a bytets32 array. Each position has the oracle address and the rate where 12 bytes are for the rate and 20 bytes are for the address. This encoded is being performed outside of the contract and error-prone for non-experienced developers or non-trained operators. Consider adding a new method which based on an array of addresses and uints 96, returns an array of bytes32 or consume more gas by changing the input parameter of provideMultiple to two different arrays.

  • Possible infinite loop if provide or provideMultiple receives by mistake the OracleFactory address as a MultiSourceOracle. Doing require(_oracle != address(this), "_oracle should be different from this") or a re-entrtancy ward as suggested above will prevent it.

General Notes

  • Dev notation is missing in the contracts: function responsibility, notes, params, and return values.

September 6, 2019. Ignacio Mazzara

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment