Uma Audit – 6. faza

Predstavitev

UMA je platforma, ki uporabnikom omogoča vpisovanje finančnih pogodb z minimalnim zaupanjem v verigo blokov Ethereum. Prej smo revidirali decentraliziran orakelj, posebno predlogo finančne pogodbe, nekaj ad hoc zahtev za vleko, predlogo Perpetual Multiparty, različne inkrementalne zahteve za vleko v daljšem sodelovanju in the insured bridge.

In this audit we reviewed a new governance proposal contract, a mechanism to extend the UMA ecosystem across multiple chains, a mechanism to distribute rewards to ERC721 token holders in accordance with an off-chain specification, and an update to the insured bridge to support WETH on the Optimism chain.

Revidirana potrditev je 0c4cea3c3d5e48da6f8984b8ba3afdfea4ce47cc in obseg vključuje naslednje pogodbe:

• oracle/implementation/Proposer.sol
• cross-chain-oracle/* (excluding test and Polygon contracts)
• financial-templates/optimistic-rewarder/* (excluding test contracts)

Pregledali smo tudi spremembe datotek trdnosti v Zahteva za vlečenje 3611.

Predpostavljalo se je, da vse zunanje kode in pogodbene odvisnosti delujejo, kot je dokumentirano.

Pregled sistema

Sedanji Governance contract allows the Risk Labs Foundation to propose new governance actions that can be ratified by the UMA token holders. The new Proposer contract is intended to take the proposer role, allowing anyone to make new proposals as long as they provide a bond that will be sacrificed if the proposal fails. There is no specific incentive for making proposals. The intention is to ensure that only the actions which are very likely to be accepted will be proposed.

The new cross-chain mechanism allows governance proposal to be passed from the Ethereum mainnet to the Optimism and Arbitrum chains. In this way, the UMA governance mechanism on layer 1 can be used to govern UMA contracts on the supported layer 2 chains. The mechanism also allows price requests and resolutions to be forwarded between the layers, so Optimistic Oracles on the layer 2 chains can be secured by the mainnet Data Verification Mechanism in the same way that the layer 1 Optimistic Oracle is secured by the DVM.

It is worth noting that these messages are sent using the native bridge mechanics, which means they are limited by the characteristics of the relevant layer 2 chains. In particular, the messages from layer 2 to layer 1 could take a week or longer to transit the bridge. Moreover, the UMA governance mechanism supports proposals that include multiple ordered transactions, but this merely restricts the order in which they can be added to the bridge. It’s possible for some of those transactions to be executed in a different order, or not at all, on layer 2.

The Optimistic Rewarder contract simply mints ERC721 tokens for anyone who requests them. It also allows anyone to associate arbitrary data with any token, and to deposit various ERC20 tokens to be distributed as rewards. The interpretation of the arbitrary data and the expected distribution of rewards amongst the token holders are determined using an unspecified off-chain procedure. Anyone can make a claim that a specific ERC721 token is owed a set of rewards if they’re willing to deposit a bond. The standard Optimistic Oracle mechanism is used to allow someone else to dispute the claim, which will be resolved by the DVM. Claims that are not disputed in time are assumed to be valid, and the contract distributes the rewards accordingly. The only restriction (to simplify accounting) is that the oracle bond token cannot be used as a reward token.

Lastly, PR3611 modifies the insured bridge mechanism to avoid sending WETH across the Optimism token bridge, which is not supported. Instead, any L2 WETH deposited in the Optimism deposit box is unwrapped to L2 ETH before transiting the bridge. On layer 1, the ETH is converted to WETH before being forwarded to the WETH bridge pool.

Kritična resnost

[C01] Cannot dispute invalid reward

When disputing a reward request, the OptimisticRewardBase contract first triggers a proposal o SkinnyOptimisticOracle in nato disputes that proposal. However, the proposal sets the expiration time as an offset from the current (dispute) time, while the dispute specifies the expiration as an offset from the time of the original reward request. In most cases, this discrepancy will prevent the oracle from identifying the proposal to be disputed, which means valid disputes will not be processed and invalid reward requests will be accepted.

Consider updating the dispute invocation to correctly specify the proposal to be disputed.

Posodobitev: Popravljeno ob objavi 9e15557 in PR3690.

[C02] Repeatedly resolve proposals

O resolveProposal funkcija Proposer Naročilo simply validates that the oracle has resolved, but does not check if the bond has been distributed. This means the same proposal can be resolved multiple times, resulting in duplicate bond payments. Consider flagging or deleting existing proposals when they are resolved.

Posodobitev: Popravljeno ob objavi b152718 in PR3689.

Visoka resnost

Jih ni.

Srednja resnost

[M01] Incorrect event parameters

O OptimisticRewarderBase contract defines a Requested dogodek which is emitted from the requestRedemption function when a redemption is requested. This event is defined to emit the expiry time of the redemption as its last parameter. However, when the event is emitted, its last parameter is incorrectly set to the Trenutni čas.

Similarly the Redeemed dogodek reads the expiry time after the record is deleted, so it will be incorrectly set to zero.

Given that this event can be used to trigger off-chain computations, consider updating the emitted value appropriately.

Posodobitev: Popravljeno ob objavi f04eef9 in PR3694.

Nizka resnost

[L01] Lack of event emission after disputing a redemption

O OptimisticRewarderBase contract defines a Disputed dogodek that is intended to be triggered if a redemption is disputed. However, this event is not emitted within or outside of the OptimisticRewarderBase pogodba.

Consider emitting the event after sensitive changes take place in the dispute funkcija, to facilitate tracking and notify off-chain clients following the contracts’ activity.

Posodobitev: Popravljeno ob objavi c275e92 in PR3695.

[L02] Inconsistent reentrancy guard

O Optimism_ParentMessenger in Arbitrum_ParentMessenger contracts inconsistently apply the nonReentrant modifier. Consider including it on all public functions.

Posodobitev: Popravljeno ob objavi 6275c39 in PR3677.

[L03] Misleading comments

Here are some misleading comments we identified during our review:

• ChildMessengerConsumerInterface.sol:
  • linija 5 says “parent messenger” instead of “child messenger”
• GovernorSpoke.sol:
  • Vrstice 49-51 povezave do a Gnosis file even though the comment says the snippet was copied from Governor.sol. Additionally, the snippet is not identical to the one in Governor.sol

Posodobitev: Popravljeno ob objavi cc350f9 in PR3678.

[L04] Missing ancillary data stamp

When requesting a price on the OracleSpoke contract, the provided ancillary data je žigosana with the child chain identifier. However, the hasPrice in getPrice functions don’t stamp the ancillary data when identifying the price request. This forces calling contracts to apply the stamp themselves, which causes an inconsistency between the price request and price retrieval mechanisms. Consider applying the stamp in the hasPrice in getPrice funkcije.

Posodobitev: Popravljeno ob objavi fdb845d in PR3668.

[L05] Missing NatSpec parameter

Many functions in the OptimisticRewarderBase Naročilo are missing the @return parameter in their Natural Specification comments. Consider including it for completeness.

Posodobitev: Popravljeno ob objavi 8920f38 in PR3679.

[L06] Residual allowance

In order to invoke the Optimistic Oracle, the OptimisticRewarderBase Naročilo grants it a token allowance, so it can pull the bond payments. If the proposal fails, the reward redemption is cancelled but the allowance is not reset. Therefore, the Optimistic Oracle will retain an unnecessary residual allowance until the next time a dispute is triggered. Consider revoking the allowance if the proposal fails.

Posodobitev: Popravljeno ob objavi c2d444b in PR3698.

[L07] Invalid refund address

The refund L2 address of the Arbitrum_ParentMessenger is initialized to the contract owner, which should be the L1 Governor. Similarly, the setRefundL2Address has a comment stating it should be set to the governor. However, when passing messages over the bridge, this value is set as the L2 user, which is the address on Arbitrum that receives excess funds after the ticket is resolved. Since the L1 governor address will not be accessible on Arbitrum, any funds sent to this address will be lost.

Consider setting it to a valid L2 address.

Posodobitev: Popravljeno ob objavi b3f2dd1 in PR3687.

[L08] Mechanism to change child messengers

O GovernorSpoke in OracleSpoke contract each initialize the child messenger in the constructor, with no mechanism to update it. This means that when the child messenger is changed, both spoke contracts become obsolete.

Since the spoke contract are likely more stable than the messengers, consider including a mechanism to update the messenger on the spokes.

Posodobitev: Popravljeno ob objavi 7c9e061 in PR3688.

Opombe in dodatne informacije

[N01] Change bond token

O Proposer pogodba vključuje a mechanism for the owner to change the size of the proposal bond. Consider whether they should also be able to change the bond token. Note that this would require a mechanism to identify the correct bond currency when existing proposals are resolved.

Posodobitev: Not an issue. UMA’s statement for this issue:

N01 recommends enabling the proposer contract to change the bond token to something other than UMA. We have no intention of supporting any token other than $UMA for this function and so have chosen to not make any changes for this issue. Moreover, a single token per contract keeps this logic as simple as possible. Lastly, If a change was needed (in the case of a token migration, for instance), we could just deploy a new proposer contract with the other token and initiate a proposal to migrate the system to use that one.

[N02] Incomplete interface

O ChildMessengerInterface does not specify a processMessageFromCrossChainParent function, even though it is assumed to exist by parent messengers. Consider including it for completeness.

Posodobitev: Not fixed. UMA’s statement for this issue:

We intentionally chose to leave this interface inconsistent as implementing this within the ChildMessengerInterface breaks compatibility with the Polygon_ChildMessenger as Polygon’s method for processing messages from other chains requires somewhat custom logic wherein an internal method is called called _processMessageFromRoot.

[N03] Incorrect interface

O GovernorSpoke contract incorrectly uporablja ChildMessengerConsumerInterface tip to describe its messenger variable. Consider using the ChildMessengerInterface namesto tega.

Posodobitev: Popravljeno ob objavi f31a527 in PR3680.

[N04] Pull tokens to Store

V previous audit we questioned the purpose of the Store pogodbe payOracleFeesErc20 funkcija (in issue L19). The UMA team opted to keep the function to standardized the interface for potential future modifications. Since the purpose of the function is not fully specified, it is unclear whether it should be triggered when the Proposer Naročilo confiscates a bond. It likely should be used when the OracleHub pays for a price request. Consider whether the function should be used in either scenario.

Posodobitev: Acknowledged. UMA’s statement for this issue:

N04 recommends using the Store’s payOracleFeeErc20 method for paying fees in both the Proposer and OracleHub contracts to be consistent with the Store usage. We’ve opted to not use this function as it would mean needing to import an additional interface (for the store) and require casting of the bond amount to a FixedPoint (which would also require an additional import. To keep the code simple and clean we’ve opted to not do this. The OZ feedback on payOracleFeeErc20 in audit phase 1 in April 2020 was valid that this method is not really useful, making this kind of integration harder to reason about.

[N05] TODO v kodi

There are “TODO” comments in the code base that should be tracked in the project’s issues backlog. For example:

• vrstica 37 of Arbitrum_ParentMessenger Naročilo
• vrstica 25 of Optimism_ChildMessenger Naročilo
• Črte 83 in 146 of OracleHub pogodba.

During development, having well described “TODO” comments will make the process of tracking and solving them easier. Without that information, these comments might tend to rot and important information for the security of the system might be forgotten by the time it is released to production.

These TODO comments should have a brief description of the task pending to do, and a link to the corresponding issue in the project repository.

Consider updating the TODO comments to add this information. For completeness and traceability, a signature and a timestamp can be added. For example:

// TODO: point this at an interface instead.

// https://github.com/UMAprotocol/protocol/issues/XXXX

// --mrice32 - 20211209

Posodobitev: Popravljeno ob objavi 5d57b5b in PR3684.

[N06] Tiskarske napake

The codebase contains the following typographical errors:

• v Admin_ChildMessenger pogodba, impleenting moral bi biti implementing
• v OptimisticRewarderBase pogodba, timestap moral bi biti timestamp.
• v OptimisticRewarderBase pogodba, liveness liveness moral bi biti liveness.
• v GovernorSpoke pogodba, only called moral bi biti only be called.
• v Optimism_ChildMessenger pogodba:

Posodobitev: Popravljeno ob objavi 9b92b0b in PR3681.

[N07] Unused imports

To improve readability of the code, consider removing the following unused imports:

Posodobitev: Popravljeno ob objavi 40b7221 in PR3682.

[N08] L2 transaction ordering

O Governor zagotavlja transactions within a proposal are executed in order. However, when those transactions involve cross-chain transactions, this merely guarantees that they arrive at the L1 bridge contract in the correct order. In the Arbitrum case, they may be reordered before they are finalized on L2. Therefore, governance proposals should be constructed to permit the possibility of reordered L2 transactions.

Posodobitev: Popravljeno ob objavi 0fb2e7b in PR3703. GovernorHub can now relay an array of L2 transactions.

zaključek

Two critical issues were found in the codebase. One medium severity issue and several minor vulnerabilities have been found, and recommendations for fixes have been suggested.

Source: https://blog.openzeppelin.com/uma-audit-phase-6/?utm_source=rss&utm_medium=rss&utm_campaign=uma-audit-phase-6