Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eugene audit 2021 05 11 #74

Merged
merged 4 commits into from
May 12, 2021
Merged

Eugene audit 2021 05 11 #74

merged 4 commits into from
May 12, 2021

Conversation

evgenykuzyakov
Copy link
Contributor

I've decided to comment inline.

Overall I didn't find anything major. I think the biggest issue is remove_all_storage. This is a protocol level decision that led to the removal of prefix deletion or iterations. The recent protocol change also restricted account deletion if the storage usage exceeds 10Kb (beyond the contract). This is done to prevent key iteration.

    /// Removes all storage for the given address.
    pub fn remove_all_storage(_address: &Address) {
        // FIXME: there is presently no way to prefix delete trie state.
        // NOTE: There is not going to be a method on runtime for this.
        //     You may need to store all keys in a list if you want to do this in a contract.
        //     Maybe you can incentivize people to delete dead old keys. They can observe them from
        //     external indexer node and then issue special cleaning transaction.
        //     Either way you may have to store the nonce per storage address root. When the account
        //     has to be deleted the storage nonce needs to be increased, and the old nonce keys
        //     can be deleted over time. That's how TurboGeth does storage.
    }

Without an ability to remove old storage from the account may result in a storage draining in a longer run that is not covered by the protocol yet. I'd recommend addressing this issue asap, which may require a hardfork of existing addresses/contracts.

Potential implementation is:

#[allow(dead_code)]
pub fn storage_to_key_nonced(address: &Address, storage_nonce: u32, key: &H256) -> [u8; 57] {
    let mut result = [0u8; 57];
    result[0] = KeyPrefix::Storage as u8;
    result[1..21].copy_from_slice(&address.0);
    result[21..25].copy_from_slice(&storage_nonce.to_le_bytes());
    result[25..].copy_from_slice(&key.0);
    result
}

@joshuajbouw
Copy link
Contributor

Thanks @evgenykuzyakov for looking into this.

@artob artob requested a review from joshuajbouw May 12, 2021 06:49
@artob artob added C-bug Category: Something isn't working. C-docs Category: Improvements or additions to documentation C-enhancement Category: New feature or request labels May 12, 2021
@artob artob added this to In progress in EVM on MainNet via automation May 12, 2021
@artob
Copy link
Contributor

artob commented May 12, 2021

@evgenykuzyakov Thanks for that! We are addressing your comments both for the current milestone as well as the roadmap going forward.

EVM on MainNet automation moved this from In progress to Reviewer approved May 12, 2021
@artob artob merged commit 42ce8e6 into master May 12, 2021
EVM on MainNet automation moved this from Reviewer approved to Done May 12, 2021
@artob artob deleted the eugene-audit-2021-05-11 branch May 12, 2021 07:03
@@ -328,6 +330,8 @@ pub fn self_deploy(code_key: &[u8]) {
// Remove code from storage and store it in register 1.
exports::storage_remove(code_key.len() as _, code_key.as_ptr() as _, 1);
exports::promise_batch_action_deploy_contract(promise_id, u64::MAX as _, 1);
// TODO: Call upgrade on the same promise to make sure the state has migrated successfully.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evgenykuzyakov I don't understand what you mean here. Could you elaborate further?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Something isn't working. C-docs Category: Improvements or additions to documentation C-enhancement Category: New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants