Skip to content

Latest commit

 

History

History
119 lines (83 loc) · 5.56 KB

SIP-0050.md

File metadata and controls

119 lines (83 loc) · 5.56 KB
Raw
SIP Title Author Status Track Created
0050
Critical staking vulnerability fix
Tyrone Johnson (@tjcloa)
Ready for vote
Contract
2022-10-18

SIP-0050: Critical staking vulnerability fix

Description

The Staking contract has been paused to prevent malicious use of the information disclosed by this SIP.

If approved, this proposal will upgrade the Staking contract to an implementation that fixes a critical vulnerability.

Motivation

A security researcher has reported a critical vulnerability through Sovryn's Immunefi bug bounty program. The vulnerability allows any address to obtain arbitrarily high voting power that would result in an unfair advantage in the Bitocracy voting system. The Exchequer Multisig has paused staking to allow us to expose the vulnerability publicly and to vote on the deployment and acceptance of the fix, without putting our governance at risk in the meantime. It is important to note that this vulnerability has not been exploited on mainnet to date.

Details

The vulnerability

Stakers can increase their voting power by using a vulnerability in the Staking contract extendStakingDuration() by submitting specific parameter values.

The Staking::extendStakingDuration() function can be used to extend the "until" time for one's stake. It takes two parameters: the previous timestamp and the new timestamp. The function takes out the stake amounts and delegation amounts from the previous timestamp and attaches them to the new timestamp. It will also change the delegation in case you delegated to someone else (delegations are timestamp-specific). More specifically, in Staking::extendStakingDuration(), by exploiting the vulnerability it was possible to manipulate variables and set previousLock == until after verification of require(previousLock < until, "S04"); // must increase staking duration, therefore bypassing the initial verification.

Here is the logic that does not work when previousLock == until, and must therefore be fixed:

address delegateFrom = delegates[msg.sender][previousLock];
address delegateTo = delegates[msg.sender][until];
if (delegateTo == address(0)) {
    delegateTo = delegateFrom;
    delegates[msg.sender][until] = delegateFrom;
}
delegates[msg.sender][previousLock] = address(0);
_decreaseDelegateStake(delegateFrom, previousLock, amount);
_increaseDelegateStake(delegateTo, until, amount);

The stake and delegate balances do indeed decrease and increase correctly. But the line delegates[msg.sender][previousLock] = address(0) zeroes out the delegation of the previousLock timestamp, which is the same as the new timestamp.

Now that the delegation address for the currently set staking timestamp is address(0), the attacker can call the delegate() function, which will delegate their entire balance as delegateStake to any address, but without decreasing the delegator's delegateStake balance. By repeating this call, the attacker can continuously increase an address' delegateStake without limit (because source voting power is not getting properly decreased), resulting in arbitrarily high voting power.

The fix

We introduce two small changes:

Move line 143 of Staking.sol require(previousLock < until, "S04"); // must increase staking duration to line 150.

This:

 function extendStakingDuration(uint256 previousLock, uint256 until) public whenNotPaused {
        until = timestampToLockDate(until);
        require(previousLock < until, "S04"); // must increase staking duration

        _notSameBlockAsStakingCheckpoint(previousLock);

        /// @dev Do not exceed the max duration, no overflow possible.
        uint256 latest = timestampToLockDate(block.timestamp + MAX_DURATION);
        if (until > latest) until = latest;

is changed to this:

 function extendStakingDuration(uint256 previousLock, uint256 until) public whenNotPaused {
        until = timestampToLockDate(until);

        _notSameBlockAsStakingCheckpoint(previousLock);

        /// @dev Do not exceed the max duration, no overflow possible.
        uint256 latest = timestampToLockDate(block.timestamp + MAX_DURATION);
        if (until > latest) until = latest;
        require(previousLock < until, "S04"); // must increase staking duration

Move line 173 of Staking.sol (delegates[msg.sender][previousLock] = address(0);) between line 167 and 168.

This:

        address delegateFrom = delegates[msg.sender][previousLock];
        address delegateTo = delegates[msg.sender][until];
        if (delegateTo == address(0)) {
            delegateTo = delegateFrom;
            delegates[msg.sender][until] = delegateFrom;
        }
        delegates[msg.sender][previousLock] = address(0);

is changed to this:

        address delegateFrom = delegates[msg.sender][previousLock];
        delegates[msg.sender][previousLock] = address(0);
        address delegateTo = delegates[msg.sender][until];
        if (delegateTo == address(0)) {
            delegateTo = delegateFrom;
            delegates[msg.sender][until] = delegateFrom;
        }

Both changes individually fix the bug and prevent the delegate address from ever being zeroed out during the staking extensions, thereby closing the vulnerability.

Proposed change

Existing Staking Logic contract: 0x81570497e763809900A8e91c8DaF3020085e43aB
New Staking Logic contract: 0xA0a44943f84f1766b42e67273EE6091A6ccc9315
Proposed changes: DistributedCollective/Sovryn-smart-contracts#454

License

Copyright and related rights waived via CC0.