| SIP | Title | Author | Status | Track | Created |
|---|---|---|---|---|---|
0043 |
Critical governance bug fix |
Hakū (@hakuryuuhakuu), Tyrone Johnson (@tjcloa) |
Approved |
Contract |
2022-03-27 |
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 bug.
A security researcher has reported a bug through Sovryn's Immunefi bug bounty program, and we have graded it as critical. The bug allows any address to obtain arbitrarily high voting power that would result in unfair voting weightings. The Exchequer Multisig has paused staking to allow us to expose the bug 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 bug has not been exploited on mainnet to date.
Stakers can delegate their voting power to other users through the Staking contract.
Staking rewards depend on the duration of the stake commitment, which can be extended in 14 days increments.
The Staking::extendStakingDuration() function can be used to extend the "until" time for one's stake. It takes 2 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():
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);
However, the logic does not work when previousLock == until. 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.
Change line 138 of Staking.sol from:
require(previousLock <= until, "cannot reduce the staking duration")
to:
require(previousLock < until, "must increase the staking duration")
This prevents the delegate address from ever being zeroed out during extensions, and closes the vulnerability.
Existing Staking Logic contract: 0xe31A938F5cf1C41747B5F20f9dD5d509B2ACd49c
New Staking Logic contract: 0x4769c04F2537C3b951Efa8a330A15716B5913Af6
Proposed changes: DistributedCollective/Sovryn-smart-contracts#427
Copyright and related rights waived via CC0.