search

anoma / namada

Rust implementation of Namada, a Proof-of-Stake L1 for interchain asset-agnostic privacy
https://namada.net
GNU General Public License v3.0
2.38k stars 950 forks source link

Incorrect accounting of voting power for Ethereum events (audit issue #5) #556

Closed james-chf closed 1 year ago

james-chf commented 1 year ago

This was finding 5 from the audit of eth-bridge-integration branch (commit 57fc202ea33df5072c9286f5ccb339537b1be885)

Severity: Major In apps/src/lib/node/ledger/shell/vote_extensions/ethereum_events.rs: 186, the voting power for events gets accounted for cumulatively rather than per event to check whether sufficient votes are provided for Ethereum events by Namada validators. This allows malicious validators to successfully add invalid events. For example, an event crafted without actually locking funds in the Ethereum smart contract could be used to mint new funds on the Namada chain. Recommendation We recommend checking that the voting power of every seen Ethereum event is greater than 2⁄3 of the total votes.

james-chf commented 1 year ago

At this point in the code:

https://github.com/anoma/namada/blob/595d14702670500e3231daa55c6e1fdbe13a34cc/apps/src/lib/node/ledger/shell/vote_extensions/ethereum_events.rs#L184-L189

The voting power being summed here is the block proposer checking that they are including at least >2/3 vote extensions by voting power from the previous consensus round in the digest they are making. A block proposer must have received at least >2/3 vote extensions by voting power, so if they don't include at least that amount's worth of vote extensions in their digest, then they're engaging in censorship and their block could (and should) be rejected by other validators.

The relevant block of code in the latest version of eth-bridge-integration at the time of writing is here: https://github.com/anoma/namada/blob/41c9011e6603cf15fc660ed9453acf675f5db3ab/apps/src/lib/node/ledger/shell/vote_extensions/eth_events.rs#L225-L237

The design has changed since the audit so this code is not enabled, as we will not be using Tendermint ABCI++ for initial launch - there is no guarantee on what "vote extensions" a block proposer may have available during PrepareProposal, as vote extensions are not tied to consensus anymore but are instead submitted as regular transactions via the Tendermint mempool. We should eventually move back towards this design though once Tendermint ABCI++ is finished.

When we move back to the ABCI++ design, we should make this code clearer e.g. by renaming variables or adding comments, and separating out the calculation of this voting power from the for-loop it is in which is also constructing an Ethereum events related digest. The main thing to make clearer is that the voting power being summed here is for the block proposer to check that they are not constructing an invalid block that will be rejected by other validators. In some sense, this should never happen in any case as a block proposer's Tendermint should not have been able to commit the previous block, if it never received at least >2/3 of vote extensions (i.e. precommits) by voting power.

james-chf commented 1 year ago

Leaving this GH issue open as the code around where this issue was found could be made clearer

cwgoes commented 1 year ago

@batconjurer @james-chf Can someone add a comment to the code and close this?