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.

Sign up for GitHub

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

Jump to bottom

[C03] Updating or removing a service type causes critical accounting errors #555

Merged
merged 5 commits into from
Jun 25, 2020
Merged

[C03] Updating or removing a service type causes critical accounting errors #555

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a1aa29f
Minor test cleanup
SidSethi Jun 23, 2020
63338ee
Remove ServiceTypeManager.updateServiceType to remove delegation acco…
SidSethi Jun 24, 2020
c715819
Fix ServiceTypeManager.removeServiceType() logic + add tests to confi…
SidSethi Jun 24, 2020
3b7de43
Fix test race condition
SidSethi Jun 24, 2020
80ff068
Ensure serviceType cannot be re-added + Validate stake bounds on serv…
SidSethi Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions eth-contracts/contracts/ServiceProviderFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,9 @@ contract ServiceProviderFactory is InitializableV2 {
);

// Update min and max totals for this service provider
(uint typeMin, uint typeMax) = ServiceTypeManager(
(, uint typeMin, uint typeMax) = ServiceTypeManager(
serviceTypeManagerAddress
).getServiceTypeStakeInfo(_serviceType);
).getServiceTypeInfo(_serviceType);
hareeshnagaraj marked this conversation as resolved.
Show resolved Hide resolved
spDetails[msg.sender].minAccountStake = spDetails[msg.sender].minAccountStake.add(typeMin);
spDetails[msg.sender].maxAccountStake = spDetails[msg.sender].maxAccountStake.add(typeMax);

Expand Down Expand Up @@ -281,9 +281,9 @@ contract ServiceProviderFactory is InitializableV2 {
spDetails[msg.sender].numberOfEndpoints -= 1;

// Update min and max totals for this service provider
(uint typeMin, uint typeMax) = ServiceTypeManager(
(, uint typeMin, uint typeMax) = ServiceTypeManager(
serviceTypeManagerAddress
).getServiceTypeStakeInfo(_serviceType);
).getServiceTypeInfo(_serviceType);
spDetails[msg.sender].minAccountStake = spDetails[msg.sender].minAccountStake.sub(typeMin);
spDetails[msg.sender].maxAccountStake = spDetails[msg.sender].maxAccountStake.sub(typeMax);

Expand Down
57 changes: 18 additions & 39 deletions eth-contracts/contracts/ServiceTypeManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,15 @@ contract ServiceTypeManager is InitializableV2 {
/// @dev List of valid service types
bytes32[] private validServiceTypes;

/// @dev Struct representing service type stake requirements
struct ServiceTypeStakeRequirements {
/// @dev Struct representing service type info
struct ServiceTypeInfo {
bool isValid;
uint minStake;
uint maxStake;
}

/// @dev mapping of service type to registered requirements
mapping(bytes32 => ServiceTypeStakeRequirements) serviceTypeStakeRequirements;
/// @dev mapping of service type info
mapping(bytes32 => ServiceTypeInfo) serviceTypeInfo;

event SetServiceVersion(bytes32 _serviceType, bytes32 _serviceVersion);
event Test(string msg, bool value);
Expand Down Expand Up @@ -80,7 +81,8 @@ contract ServiceTypeManager is InitializableV2 {
require(!this.serviceTypeIsValid(_serviceType), "Already known service type");

validServiceTypes.push(_serviceType);
serviceTypeStakeRequirements[_serviceType] = ServiceTypeStakeRequirements({
serviceTypeInfo[_serviceType] = ServiceTypeInfo({
isValid: true,
minStake: _serviceTypeMin,
maxStake: _serviceTypeMax
});
Expand Down Expand Up @@ -109,46 +111,23 @@ contract ServiceTypeManager is InitializableV2 {
uint lastIndex = validServiceTypes.length - 1;
validServiceTypes[serviceIndex] = validServiceTypes[lastIndex];
validServiceTypes.length--;
// Overwrite values
serviceTypeStakeRequirements[_serviceType].minStake = 0;
serviceTypeStakeRequirements[_serviceType].maxStake = 0;
}

/**
* @notice Update a service type
* @param _serviceType - type of service
* @param _serviceTypeMin - minimum stake for service type
* @param _serviceTypeMax - maximum stake for service type
*/
function updateServiceType(
bytes32 _serviceType,
uint _serviceTypeMin,
uint _serviceTypeMax
) external
{
_requireIsInitialized();
require(
msg.sender == governanceAddress,
"Only callable by Governance contract"
);

require(this.serviceTypeIsValid(_serviceType), "Invalid service type");

serviceTypeStakeRequirements[_serviceType].minStake = _serviceTypeMin;
serviceTypeStakeRequirements[_serviceType].maxStake = _serviceTypeMax;
// Mark as invalid
serviceTypeInfo[_serviceType].isValid = false;
hareeshnagaraj marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @notice Get min and max stake for a given service type
* @notice Get isValid, min and max stake for a given service type
* @param _serviceType - type of service
* @return min and max stake for type
* @return isValid, min and max stake for type
*/
function getServiceTypeStakeInfo(bytes32 _serviceType)
external view returns (uint min, uint max)
function getServiceTypeInfo(bytes32 _serviceType)
external view returns (bool isValid, uint minStake, uint maxStake)
{
return (
serviceTypeStakeRequirements[_serviceType].minStake,
serviceTypeStakeRequirements[_serviceType].maxStake
serviceTypeInfo[_serviceType].isValid,
serviceTypeInfo[_serviceType].minStake,
serviceTypeInfo[_serviceType].maxStake
);
}

Expand All @@ -167,7 +146,7 @@ contract ServiceTypeManager is InitializableV2 {
function serviceTypeIsValid(bytes32 _serviceType)
external view returns (bool isValid)
{
return serviceTypeStakeRequirements[_serviceType].maxStake > 0;
return serviceTypeInfo[_serviceType].isValid;
}

// ========================================= Service Version Logic =========================================
Expand All @@ -191,7 +170,7 @@ contract ServiceTypeManager is InitializableV2 {
"Already registered"
);

// Update array of known types
// Update array of known versions for type
serviceTypeVersions[_serviceType].push(_serviceVersion);

// Update status for this specific service version
Expand Down
14 changes: 6 additions & 8 deletions eth-contracts/migrations/7_versioning_service_migration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,9 @@ module.exports = (deployer, network, accounts) => {
callDataCN,
{ from: guardianAddress }
)
const serviceTypeCNStakeInfo = await serviceTypeManager.getServiceTypeStakeInfo.call(serviceTypeCN)
const [cnTypeMinV, cnTypeMaxV] = [serviceTypeCNStakeInfo[0], serviceTypeCNStakeInfo[1]]
assert.ok(_lib.toBN(cnTypeMin).eq(cnTypeMinV), 'Expected same minStake')
assert.ok(_lib.toBN(cnTypeMax).eq(cnTypeMaxV), 'Expected same max Stake')
const serviceTypeCNInfo = await serviceTypeManager.getServiceTypeInfo.call(serviceTypeCN)
assert.ok(_lib.toBN(cnTypeMin).eq(serviceTypeCNInfo.minStake), 'Expected same minStake')
assert.ok(_lib.toBN(cnTypeMax).eq(serviceTypeCNInfo.maxStake), 'Expected same max Stake')

const callDataDP = _lib.abiEncode(
['bytes32', 'uint256', 'uint256'],
Expand All @@ -98,10 +97,9 @@ module.exports = (deployer, network, accounts) => {
callDataDP,
{ from: guardianAddress }
)
const serviceTypeDPStakeInfo = await serviceTypeManager.getServiceTypeStakeInfo.call(serviceTypeDP)
const [dpTypeMinV, dpTypeMaxV] = [serviceTypeDPStakeInfo[0], serviceTypeDPStakeInfo[1]]
assert.ok(_lib.toBN(dpTypeMin).eq(dpTypeMinV), 'Expected same minStake')
assert.ok(_lib.toBN(dpTypeMax).eq(dpTypeMaxV), 'Expected same maxStake')
const serviceTypeDPInfo = await serviceTypeManager.getServiceTypeInfo.call(serviceTypeDP)
assert.ok(_lib.toBN(dpTypeMin).eq(serviceTypeDPInfo.minStake), 'Expected same minStake')
assert.ok(_lib.toBN(dpTypeMax).eq(serviceTypeDPInfo.maxStake), 'Expected same maxStake')

// Deploy ServiceProviderFactory logic and proxy contracts + register proxy
const serviceProviderFactory0 = await deployer.deploy(ServiceProviderFactory, { from: proxyDeployerAddress })
Expand Down
115 changes: 111 additions & 4 deletions eth-contracts/test/delegateManager.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const delegateManagerKey = web3.utils.utf8ToHex('DelegateManager')
const tokenRegKey = web3.utils.utf8ToHex('Token')

const testDiscProvType = web3.utils.utf8ToHex('discovery-provider')
const serviceTypeMinStake = _lib.audToWei(5)
const serviceTypeMaxStake = _lib.audToWei(10000000)
const testEndpoint = 'https://localhost:5000'
const testEndpoint1 = 'https://localhost:5001'
const testEndpoint3 = 'https://localhost:5002'
Expand All @@ -32,7 +34,7 @@ const callValue0 = _lib.toBN(0)

contract('DelegateManager', async (accounts) => {
let staking, stakingAddress, token, registry, governance, claimsManager0, claimsManagerProxy
let serviceProviderFactory, claimsManager, delegateManager
let serviceProviderFactory, serviceTypeManager, claimsManager, delegateManager

// intentionally not using acct0 to make sure no TX accidentally succeeds without specifying sender
const [, proxyAdminAddress, proxyDeployerAddress] = accounts
Expand All @@ -43,6 +45,10 @@ contract('DelegateManager', async (accounts) => {
const delegatorAccount1 = accounts[11]
const slasherAccount = stakerAccount

/**
* Initialize Registry, Governance, Token, Staking, ServiceTypeManager, ServiceProviderFactory, ClaimsManager, DelegateManager
* Register discprov serviceType
*/
beforeEach(async () => {
registry = await _lib.deployRegistry(artifacts, proxyAdminAddress, proxyDeployerAddress)

Expand Down Expand Up @@ -99,10 +105,10 @@ contract('DelegateManager', async (accounts) => {
{ from: proxyDeployerAddress }
)
await registry.addContract(serviceTypeManagerProxyKey, serviceTypeManagerProxy.address, { from: proxyDeployerAddress })
let serviceTypeManager = await ServiceTypeManager.at(serviceTypeManagerProxy.address)
serviceTypeManager = await ServiceTypeManager.at(serviceTypeManagerProxy.address)

// Register discprov serviceType
await _lib.addServiceType(testDiscProvType, _lib.audToWei(5), _lib.audToWei(10000000), governance, guardianAddress, serviceTypeManagerProxyKey)
await _lib.addServiceType(testDiscProvType, serviceTypeMinStake, serviceTypeMaxStake, governance, guardianAddress, serviceTypeManagerProxyKey)

// Deploy ServiceProviderFactory
let serviceProviderFactory0 = await ServiceProviderFactory.new({ from: proxyDeployerAddress })
Expand Down Expand Up @@ -311,6 +317,11 @@ contract('DelegateManager', async (accounts) => {
describe('Delegation tests', () => {
let regTx

/**
* Transfer tokens to stakers & delegator
* Register service providers
* Update SP deployer cuts
*/
beforeEach(async () => {
// Transfer 1000 tokens to stakers
await token.transfer(stakerAccount, INITIAL_BAL, { from: proxyDeployerAddress })
Expand Down Expand Up @@ -467,7 +478,7 @@ contract('DelegateManager', async (accounts) => {
await time.advanceBlockTo(undelegateRequestInfo.lockupExpiryBlock)

// Undelegate stake
delegateManager.undelegateStake({ from: delegatorAccount1 })
await delegateManager.undelegateStake({ from: delegatorAccount1 })

// Confirm all state change operations have occurred
undelegateRequestInfo = await delegateManager.getPendingUndelegateRequest(delegatorAccount1)
Expand Down Expand Up @@ -1537,5 +1548,101 @@ contract('DelegateManager', async (accounts) => {
)
})
})

it('Undelegate after serviceType removal', async () => {
/**
* Confirm initial state of serviceType and serviceProvider
* Delegate stake to Staker
* Remove serviceType
* Confirm new state of serviceType and serviceProvider
* Deregister SP of serviceType
* Undelegate stake from Staker
* Confirm new delegation state
*/

const minStakeBN = _lib.toBN(serviceTypeMinStake)
const maxStakeBN = _lib.toBN(serviceTypeMaxStake)

// Confirm initial serviceType info
const stakeInfo0 = await serviceTypeManager.getServiceTypeInfo.call(testDiscProvType)
assert.isTrue(stakeInfo0.isValid, 'Expected isValid == true')
assert.isTrue(stakeInfo0.minStake.eq(minStakeBN), 'Expected same minStake')
assert.isTrue(stakeInfo0.maxStake.eq(maxStakeBN), 'Expected same maxStake')

// Confirm initial SP details
const spDetails0 = await serviceProviderFactory.getServiceProviderDetails.call(stakerAccount)
assert.isTrue(spDetails0.deployerStake.eq(DEFAULT_AMOUNT), 'Expected deployerStake == default amount')
assert.isTrue(spDetails0.validBounds, 'Expected validBounds == true')
assert.isTrue(spDetails0.numberOfEndpoints.eq(_lib.toBN(1)), 'Expected one endpoint')
assert.isTrue(spDetails0.minAccountStake.eq(minStakeBN), 'Expected minAccountStake == dpTypeMin')
assert.isTrue(spDetails0.maxAccountStake.eq(maxStakeBN), 'Expected maxAccountStake == dpTypeMax')

// Delegate stake to Staker
const delegationAmount = _lib.audToWeiBN(60)
await token.approve(
stakingAddress,
delegationAmount,
{ from: delegatorAccount1 }
)
await delegateManager.delegateStake(
stakerAccount,
delegationAmount,
{ from: delegatorAccount1 }
)

// Remove serviceType
await governance.guardianExecuteTransaction(
serviceTypeManagerProxyKey,
callValue0,
'removeServiceType(bytes32)',
_lib.abiEncode(['bytes32'], [testDiscProvType]),
{ from: guardianAddress }
)

// Confirm serviceType info is changed after serviceType removal
const stakeInfo1 = await serviceTypeManager.getServiceTypeInfo.call(testDiscProvType)
assert.isFalse(stakeInfo1.isValid, 'Expected isValid == false')
assert.isTrue(stakeInfo1.minStake.eq(minStakeBN), 'Expected same minStake')
assert.isTrue(stakeInfo1.maxStake.eq(maxStakeBN), 'Expected same maxStake')

// Confirm SP details are unchanged after serviceType removal
const spDetails = await serviceProviderFactory.getServiceProviderDetails.call(stakerAccount)
hareeshnagaraj marked this conversation as resolved.
Show resolved Hide resolved
assert.isTrue(spDetails.deployerStake.eq(DEFAULT_AMOUNT), 'Expected deployerStake == default amount')
assert.isTrue(spDetails.validBounds, 'Expected validBounds == true')
assert.isTrue(spDetails.numberOfEndpoints.eq(_lib.toBN(1)), 'Expected one endpoint')
assert.isTrue(spDetails.minAccountStake.eq(minStakeBN), 'Expected minAccountStake == dpTypeMin')
assert.isTrue(spDetails.maxAccountStake.eq(maxStakeBN), 'Expected maxAccountStake == dpTypeMax')

// Deregister SP + unstake
await _lib.deregisterServiceProvider(
serviceProviderFactory,
testDiscProvType,
testEndpoint,
stakerAccount
)
const deregisterRequestInfo = await serviceProviderFactory.getPendingDecreaseStakeRequest.call(stakerAccount)
await time.advanceBlockTo(deregisterRequestInfo.lockupExpiryBlock)
await serviceProviderFactory.decreaseStake({ from: stakerAccount })

// Undelegate all
await delegateManager.requestUndelegateStake(
stakerAccount,
delegationAmount,
{ from: delegatorAccount1 }
)
const undelegateRequestInfo = await delegateManager.getPendingUndelegateRequest(delegatorAccount1)
await time.advanceBlockTo(undelegateRequestInfo.lockupExpiryBlock)
await delegateManager.undelegateStake({ from: delegatorAccount1 })

// Confirm delegation state
const totalStakedForSP = await staking.totalStakedFor(stakerAccount)
assert.isTrue(totalStakedForSP.isZero(), 'Expected totalStaked for SP == 0')
const delegators = await delegateManager.getDelegatorsList(stakerAccount)
assert.equal(delegators.length, 0, 'Expect delegators list length == 0')
const delegatedStake = await delegateManager.getTotalDelegatorStake(delegatorAccount1)
assert.isTrue(delegatedStake.isZero(), 'Expected delegatedStake == 0')
const totalLockedDelegationForSP = await delegateManager.getTotalLockedDelegationForServiceProvider(stakerAccount)
assert.isTrue(totalLockedDelegationForSP.isZero(), 'Expected totalLockedDelegationForSP == 0')
})
})
})
22 changes: 11 additions & 11 deletions eth-contracts/test/governance.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1415,16 +1415,16 @@ contract('Governance.sol', async (accounts) => {

it('Transfer guardianship', async () => {
const newGuardianAddress = accounts[19]
const newSpMinStake = spMinStake + 2
const newSpMaxStake = spMaxStake + 2
const serviceVersion1 = web3.utils.utf8ToHex("0.0.1")
const serviceVersion2 = web3.utils.utf8ToHex("0.0.2")

// Confirm current guardianAddress is active
assert.equal(await governance.getGuardianAddress(), guardianAddress, 'Expected same guardianAddress')
await governance.guardianExecuteTransaction(
serviceTypeManagerProxyKey,
callValue0,
'updateServiceType(bytes32,uint256,uint256)',
_lib.abiEncode(['bytes32', 'uint256', 'uint256'], [testDiscProvType, newSpMinStake, newSpMaxStake]),
'setServiceVersion(bytes32,bytes32)',
_lib.abiEncode(['bytes32', 'bytes32'], [testDiscProvType, serviceVersion1]),
{ from: guardianAddress }
)

Expand All @@ -1433,8 +1433,8 @@ contract('Governance.sol', async (accounts) => {
governance.guardianExecuteTransaction(
serviceTypeManagerProxyKey,
callValue0,
'updateServiceType(bytes32,uint256,uint256)',
_lib.abiEncode(['bytes32', 'uint256', 'uint256'], [testDiscProvType, newSpMinStake, newSpMaxStake]),
'setServiceVersion(bytes32,bytes32)',
_lib.abiEncode(['bytes32', 'bytes32'], [testDiscProvType, serviceVersion2]),
{ from: newGuardianAddress }
),
"Governance::guardianExecuteTransaction: Only guardian."
Expand All @@ -1455,8 +1455,8 @@ contract('Governance.sol', async (accounts) => {
governance.guardianExecuteTransaction(
serviceTypeManagerProxyKey,
callValue0,
'updateServiceType(bytes32,uint256,uint256)',
_lib.abiEncode(['bytes32', 'uint256', 'uint256'], [testDiscProvType, newSpMinStake, newSpMaxStake]),
'setServiceVersion(bytes32,bytes32)',
_lib.abiEncode(['bytes32', 'bytes32'], [testDiscProvType, serviceVersion2]),
{ from: guardianAddress }
),
"Governance::guardianExecuteTransaction: Only guardian."
Expand All @@ -1466,8 +1466,8 @@ contract('Governance.sol', async (accounts) => {
await governance.guardianExecuteTransaction(
serviceTypeManagerProxyKey,
callValue0,
'updateServiceType(bytes32,uint256,uint256)',
_lib.abiEncode(['bytes32', 'uint256', 'uint256'], [testDiscProvType, newSpMinStake, newSpMaxStake]),
'setServiceVersion(bytes32,bytes32)',
_lib.abiEncode(['bytes32', 'bytes32'], [testDiscProvType, serviceVersion2]),
{ from: newGuardianAddress }
)
})
Expand Down Expand Up @@ -1676,7 +1676,7 @@ contract('Governance.sol', async (accounts) => {
await registryProxy.setAudiusGovernanceAddress(governance.address, { from: proxyAdminAddress })

// Upgrade registry proxy to new logic address
governance.guardianExecuteTransaction(
await governance.guardianExecuteTransaction(
registryRegKey,
callValue0,
'upgradeTo(address)',
Expand Down