WalletScheme bug

Last Friday we found an issue with the new wallet schemes that we have been using in DXvote. The issue was that we could not edit the voting parameters for WalletSchemes once they had been initialised.

Intro to new schemes

The new schemes being used here are the DXdao developed versions specifically for DXvote. These make a number of improvements and have additional security features and flexibility. Since these contracts have only been audited once - and with limited scope (dxdao-contracts/docs at main · DXgovernance/dxdao-contracts · GitHub) we are testing in a slow rollout gradually increasing the funds handled and therefore risk as we gain more confidence with them.

We first tested these of course in testnets like rinkeby and arbitrum testnet, then in production arbitrum where they have been running for a few months now.

We recently registered the first new wallet scheme on xDAI, known as the Quick Wallet Scheme (QWS).

QWS in xDAI

The reason and utility for testing in xdai with a QWS (Contract 0x983e0c64088E48b6AB7C76a8ABa3eE93d1C10aD5 - xDai Explorer) is due to the detached nature of the new schemes. These can be configured to act completely separated from the main avatar’s treasury and has its own funds as well as additional security around max mintable REP etc.

This new QWS was successfully tested in xDAI using new contributor proposal creation UI in DXvote which also launched vesting contracts for contributors alongside their usual payments and REP.

We initially set the boosted time to 2 days - this is the amount of time taken for a proposal to pass after boosting. However, we felt this was too large a change from the previous 4 days of the legacy contribution reward scheme and so made a proposal to change it to 3 days. This change went through successfully in the proposal below

https://dxvote.eth.link/#/xdai/proposal/0xe21254eb74584c8a59c3eb34ba35fc0037e9b5041f3475384722cf03f95ccd88

The issue

However upon passing a few more proposals through the scheme, we found an issue that was not immediately apparent, the times had not changed and proposals still passed after 2 days of boosting. This issue was raised, investigated and the issue found.

The bug essentially meant that once the initial parameters for schemes were set we could not change them. Meaning we couldn’t change scheme timings, thresholds, rep loss ratios, dao boosts, bounties and quorums. Important to note that if we never wanted to change anything there wouldn’t be an issue and that no funds were lost or ever at risk.

Technical explanation

On the contract side, the voting params were being set on WalletScheme initialisation in a variable and were always being fetched from the variable. However, there is no way to change this after initialisation and the call from SchemeRegistrar to change the voting params was changing a value that was never actually consumed by the WalletScheme. The change in the PR below shows now that instead of storing the voting params directly in the wallet we instead have an interface to the controller from which the editable params can be retrieved. Tests were updated to reflect this change also.

Going forward / Reflection

Given that we found this during one of the minimum risk steps of our controlled gradual roll out the impact is small. Even if the bug had risked funds it would still have been small amounts thanks to the separation of funds by the new schemes.

  1. This highlights a very clear need for additional security, defined QA testing and more audits.

This is an issue that should have been found with proper thorough testing in rinkeby and one solution to prevent this from happening again is having a list of tasks that a QA team would carry out to ensure all functions of the schemes are operating as expected.

  1. All planned movement of funds to the QWS has been cancelled and a proposal is live now to withdraw the remaining funds out

https://dxvote.eth.link/#/xdai/proposal/0xd77f0fcf8f8b367c918565b4a3e76a0464e1df5f6297f0153c72255060c94d29

  1. Version 1.1 rollout - We will test in rinkeby first then deploy the new xDAI QWS 1.1 as well as unregistering the old QWS. This issue also impacts arbitrum schemes which will need to be gradually upgraded to the new 1.1 version of the wallet scheme to enable future changes to voting parameters.

Governance call discussing this issue - DXgov Weekly Gathering [2021-12-01] - YouTube

7 Likes

Getting another audit of the WalletScheme and PermissionRegistry, and also including the DXDvotingmachine, the Controller, and the Avatar in the scope of the audit is the top priority and could be underway as early as this month.

In terms of testing, I think a lesson here is that when testing critical features the front-end should not be relied on to confirm results. In this particular example, the result of the parameter update could have been confirmed in testing by running a test proposal.

4 Likes

Somewhat related to this, I will also be posting a proposal to send 500k wxDai to the xDai MS, to be unwrapped and returned to the treasury. As most people request xDai in their proposals – and we’re running low on that. This will also leave ~185k wxDai in the xDai treasury which can be useful to moving funds to smart contract wallets.

Edit: We might actually be able to unwrap directly from the DAO, we just won’t be able to directly wrap due to native token gas limitations.

3 Likes