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

Staking Security Update - implementation for SIP-0042 #423

Merged
merged 39 commits into from
Mar 27, 2022

Conversation

tjcloa
Copy link
Contributor

@tjcloa tjcloa commented Mar 21, 2022

Implementation of SIP-0042 DistributedCollective/SIPS#42

@smbsp smbsp changed the base branch from master to development March 21, 2022 12:27
@tjcloa tjcloa assigned eMarchenko and unassigned eMarchenko Mar 21, 2022
@tjcloa tjcloa requested review from Alitasovryn and cwsnt March 21, 2022 12:36
@tjcloa tjcloa marked this pull request as ready for review March 21, 2022 13:59
Copy link

@eMarchenko eMarchenko left a comment

Choose a reason for hiding this comment

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

looks good.

you can probably save some space by delaying conversion to shorter uints to the latest possible moment, i.e.

pragma solidity 0.5.17;

library Math {
    function add96(uint96 x, uint96 y, string memory reason) internal pure returns (uint96) {
        require(x < x + y, reason);
        return x + y;
    }
}

library Cast {
    function cast96(uint x, string memory reason) internal pure returns (uint96) {
        require(x < 1<<96, reason);
        return uint96(x);
    }
}

contract A {
    uint96 public x;
    
    function foo(uint96 y) external {
        x = Math.add96(x, y, "error");
    }
}

contract B {    
    uint96 public x;
    
    function foo(uint y) external {
        x = Cast.cast96(x + y, "error");
    }
}

contract B is significantly cheaper than A. The difference increases for each short function parameter and checked arithmetic operation.

Copy link
Contributor

@HakuryuuHakuu HakuryuuHakuu left a comment

Choose a reason for hiding this comment

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

I'm on the fence on this one, but someone needs to say it and I'll gladly play devil's advocate here:
I would argue that in the spirit of "fairness" and to be on the safe side (plus being true to what is written in sip42), we should fully lock the contract in case of freeze, meaning all public non-view functions that do not bear the whenNotPaused modifier already should bear whenNotFrozen. This correctly implements the idea of a full freeze on the whole contract, not putting anyone at an advantage, not leaving any door open to any kind of exploit.
The only problem I see with this is that modifiers simply duplicate the code in each function, so we risk hitting 24k again by applying it on more functions. I do have suggestions to shave off some gas from the contract though, so let's not make this a decision factor.

This would concern 5 functions, 4 of which are protected by onlyOwner, and one by onlyAuthorized.

@smbsp smbsp changed the base branch from development to master March 22, 2022 15:17
…ezer-2

Staking-pauser-freezer-2 - no dragons and fixed pausing and freezing logic
@tjcloa tjcloa self-assigned this Mar 22, 2022
@smbsp smbsp changed the base branch from master to development March 23, 2022 09:51
@smbsp smbsp changed the base branch from development to master March 23, 2022 09:56
@tjcloa tjcloa changed the title Staking pauser freezer Staking Security Update - implementation for SIP-42 Mar 23, 2022
@tjcloa tjcloa changed the title Staking Security Update - implementation for SIP-42 Staking Security Update - implementation for SIP-0042 Mar 23, 2022
@tjcloa tjcloa merged commit 6819cf2 into master Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants