From bb1d8e01435eb43b3a697bd2a4359dea8a7504b8 Mon Sep 17 00:00:00 2001 From: Yondon Fu Date: Tue, 23 Jan 2018 10:54:52 -0500 Subject: [PATCH 1/2] Add check in transcoder() for caller to be self bonded --- contracts/bonding/BondingManager.sol | 56 ++++++++++++++------------- contracts/test/GenericMock.sol | 2 +- test/unit/BondingManager.js | 58 ++++++++++++---------------- 3 files changed, 55 insertions(+), 61 deletions(-) diff --git a/contracts/bonding/BondingManager.sol b/contracts/bonding/BondingManager.sol index cd52de3d..92a0a675 100644 --- a/contracts/bonding/BondingManager.sol +++ b/contracts/bonding/BondingManager.sol @@ -170,12 +170,17 @@ contract BondingManager is ManagerProxyTarget, IBondingManager { // Fee share must be a valid percentage require(MathUtils.validPerc(_feeShare)); + Delegator storage del = delegators[msg.sender]; + + // Must be bonded to self + require(del.delegateAddress == msg.sender); + Transcoder storage t = transcoders[msg.sender]; t.pendingBlockRewardCut = _blockRewardCut; t.pendingFeeShare = _feeShare; t.pendingPricePerSegment = _pricePerSegment; - uint256 delegatedAmount = delegators[msg.sender].delegatedAmount; + uint256 delegatedAmount = del.delegatedAmount; // Check if transcoder is not already registered if (transcoderStatus(msg.sender) == TranscoderStatus.NotRegistered) { @@ -197,29 +202,6 @@ contract BondingManager is ManagerProxyTarget, IBondingManager { TranscoderUpdate(msg.sender, _blockRewardCut, _feeShare, _pricePerSegment); } - /* - * @dev Remove the sender as a transcoder - */ - function resignAsTranscoder() - external - whenSystemNotPaused - currentRoundInitialized - { - // Sender must be registered transcoder - require(transcoderStatus(msg.sender) == TranscoderStatus.Registered); - - uint256 currentRound = roundsManager().currentRound(); - if (activeTranscoderSet[currentRound].isActive[msg.sender]) { - // If transcoder is active for the round set it as inactive - activeTranscoderSet[currentRound].isActive[msg.sender] = false; - } - - // Remove transcoder from pools - transcoderPool.remove(msg.sender); - - TranscoderResigned(msg.sender); - } - /** * @dev Delegate stake towards a specific address. * @param _amount The amount of LPT to stake. @@ -316,12 +298,18 @@ contract BondingManager is ManagerProxyTarget, IBondingManager { transcoderPool.updateKey(del.delegateAddress, transcoderPool.getKey(del.delegateAddress).sub(del.bondedAmount), address(0), address(0)); } - Unbond(del.delegateAddress, msg.sender); - // Delegator no longer bonded to anyone del.delegateAddress = address(0); // Unbonding delegator does not have a start round del.startRound = 0; + + // If caller is a registered transcoder, resign + // In the future, with partial unbonding there would be a check for 0 bonded stake as well + if (transcoderStatus(msg.sender) == TranscoderStatus.Registered) { + resignTranscoder(msg.sender); + } + + Unbond(del.delegateAddress, msg.sender); } /** @@ -804,6 +792,22 @@ contract BondingManager is ManagerProxyTarget, IBondingManager { return activeTranscoderSet[_round].isActive[_transcoder]; } + /* + * @dev Remove transcoder + */ + function resignTranscoder(address _transcoder) internal { + uint256 currentRound = roundsManager().currentRound(); + if (activeTranscoderSet[currentRound].isActive[_transcoder]) { + // If transcoder is active for the round set it as inactive + activeTranscoderSet[currentRound].isActive[_transcoder] = false; + } + + // Remove transcoder from pools + transcoderPool.remove(_transcoder); + + TranscoderResigned(_transcoder); + } + /* * @dev Update a transcoder with rewards * @param _transcoder Address of transcoder diff --git a/contracts/test/GenericMock.sol b/contracts/test/GenericMock.sol index 2cd88bea..79b63898 100644 --- a/contracts/test/GenericMock.sol +++ b/contracts/test/GenericMock.sol @@ -25,7 +25,7 @@ contract GenericMock { * @param _data Transaction data to be used to call the target contract */ function execute(address _target, bytes _data) external returns (bool) { - // solium-disable security/no-low-level-calls + // solium-disable-next-line security/no-low-level-calls return _target.call(_data); } diff --git a/test/unit/BondingManager.js b/test/unit/BondingManager.js index 6feb7224..1bafadb9 100644 --- a/test/unit/BondingManager.js +++ b/test/unit/BondingManager.js @@ -61,6 +61,14 @@ contract("BondingManager", accounts => { await expectThrow(bondingManager.transcoder(blockRewardCut, feeShare, pricePerSegment), {from: tAddr}) }) + it("should fail if transcoder is not bonded to self", async () => { + await fixture.token.setApproved(true) + await bondingManager.bond(2000, tAddr, {from: accounts[2]}) + + // Fails because transcoder has non zero delegated stake but is not bonded to self + await expectThrow(bondingManager.transcoder(blockRewardCut, feeShare, pricePerSegment), {from: tAddr}) + }) + it("should fail if blockRewardCut is an invalid percentage", async () => { const invalidBlockRewardCut = 101 * PERC_MULTIPLIER await expectThrow(bondingManager.transcoder(invalidBlockRewardCut, feeShare, pricePerSegment, {from: tAddr})) @@ -105,38 +113,6 @@ contract("BondingManager", accounts => { }) }) - describe("resignAsTranscoder", () => { - const tAddr = accounts[1] - - beforeEach(async () => { - const blockRewardCut = 10 * PERC_MULTIPLIER - const feeShare = 5 * PERC_MULTIPLIER - const pricePerSegment = 100 - - await bondingManager.setParameters(UNBONDING_PERIOD, NUM_TRANSCODERS, NUM_ACTIVE_TRANSCODERS) - - await fixture.roundsManager.setCurrentRoundInitialized(true) - await fixture.token.setApproved(true) - await bondingManager.bond(2000, tAddr, {from: tAddr}) - await bondingManager.transcoder(blockRewardCut, feeShare, pricePerSegment, {from: tAddr}) - }) - - it("should throw if transcoder is not registered", async () => { - await expectThrow(bondingManager.resignAsTranscoder({from: accounts[2]})) - }) - - it("should remove the transcoder from the transcoder pools", async () => { - await bondingManager.resignAsTranscoder({from: tAddr}) - assert.equal(await bondingManager.transcoderStatus(tAddr), 0, "transcoder not removed from pool") - }) - - it("should set a transcoder as not registered", async () => { - await bondingManager.resignAsTranscoder({from: tAddr}) - const transcoderStatus = await bondingManager.transcoderStatus(tAddr) - assert.equal(transcoderStatus, 0, "transcoder is not not registered") - }) - }) - describe("bond", () => { const tAddr0 = accounts[1] const tAddr1 = accounts[2] @@ -599,10 +575,10 @@ contract("BondingManager", accounts => { // Delegator bonds to transcoder await bondingManager.bond(2000, tAddr, {from: dAddr}) - // Set active transcoders - await fixture.roundsManager.initializeRound() // Set current round so delegator is bonded await fixture.roundsManager.setCurrentRound(currentRound) + // Set active transcoders + await fixture.roundsManager.initializeRound() }) it("should set withdraw round to current block + unbonding period", async () => { @@ -642,6 +618,20 @@ contract("BondingManager", accounts => { assert.equal(startDelegatedAmount.sub(endDelegatedAmount), 2000, "delegate's delegated amount did not decrease by bonded amount") }) + + it("should resign transcoder if caller is a registered transcoder", async () => { + await bondingManager.unbond({from: tAddr}) + + assert.equal(await bondingManager.transcoderStatus(tAddr), 0, "transcoder not removed from pool") + }) + + it("should set transcoder as inactive for the current round if caller is a registered transcoder", async () => { + assert.isOk(await bondingManager.isActiveTranscoder(tAddr, currentRound), "transcoder should be active") + + await bondingManager.unbond({from: tAddr}) + + assert.isNotOk(await bondingManager.isActiveTranscoder(tAddr, currentRound), "transcoder should be inactive") + }) }) describe("withdrawStake", () => { From dd0c9d4cefdf431fcdadf387b5f6dc6ecbad5e49 Mon Sep 17 00:00:00 2001 From: Yondon Fu Date: Tue, 23 Jan 2018 11:23:08 -0500 Subject: [PATCH 2/2] Transcoder needs non-zero bonded stake --- contracts/bonding/BondingManager.sol | 4 ++-- contracts/jobs/JobsManager.sol | 1 - test/unit/BondingManager.js | 11 +++++++---- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/contracts/bonding/BondingManager.sol b/contracts/bonding/BondingManager.sol index 92a0a675..f8eb3ad5 100644 --- a/contracts/bonding/BondingManager.sol +++ b/contracts/bonding/BondingManager.sol @@ -172,8 +172,8 @@ contract BondingManager is ManagerProxyTarget, IBondingManager { Delegator storage del = delegators[msg.sender]; - // Must be bonded to self - require(del.delegateAddress == msg.sender); + // Must have a non-zero amount bonded to self + require(del.delegateAddress == msg.sender && del.bondedAmount > 0); Transcoder storage t = transcoders[msg.sender]; t.pendingBlockRewardCut = _blockRewardCut; diff --git a/contracts/jobs/JobsManager.sol b/contracts/jobs/JobsManager.sol index 7deeb45d..943ecd56 100644 --- a/contracts/jobs/JobsManager.sol +++ b/contracts/jobs/JobsManager.sol @@ -391,7 +391,6 @@ contract JobsManager is ManagerProxyTarget, IVerifiable, IJobsManager { // Sender must be elected transcoder require(job.transcoderAddress == msg.sender); - uint256 blockNum = roundsManager().blockNum(); uint256 challengeBlock = claim.claimBlock + 1; // Segment must be eligible for verification // roundsManager().blockHash() ensures that the challenge block is within the last 256 blocks from the current block diff --git a/test/unit/BondingManager.js b/test/unit/BondingManager.js index 1bafadb9..a5e8818d 100644 --- a/test/unit/BondingManager.js +++ b/test/unit/BondingManager.js @@ -57,10 +57,6 @@ contract("BondingManager", accounts => { await expectThrow(bondingManager.transcoder(blockRewardCut, feeShare, pricePerSegment), {from: tAddr}) }) - it("should fail with zero delegated amount", async () => { - await expectThrow(bondingManager.transcoder(blockRewardCut, feeShare, pricePerSegment), {from: tAddr}) - }) - it("should fail if transcoder is not bonded to self", async () => { await fixture.token.setApproved(true) await bondingManager.bond(2000, tAddr, {from: accounts[2]}) @@ -69,6 +65,13 @@ contract("BondingManager", accounts => { await expectThrow(bondingManager.transcoder(blockRewardCut, feeShare, pricePerSegment), {from: tAddr}) }) + it("should fail if transcoder does not have a non-zero amount bonded to self", async () => { + await bondingManager.bond(0, tAddr, {from: tAddr}) + + // Fails because transcoder is delegated to self but has zero bonded stake + await expectThrow(bondingManager.transcoder(blockRewardCut, feeShare, pricePerSegment), {from: tAddr}) + }) + it("should fail if blockRewardCut is an invalid percentage", async () => { const invalidBlockRewardCut = 101 * PERC_MULTIPLIER await expectThrow(bondingManager.transcoder(invalidBlockRewardCut, feeShare, pricePerSegment, {from: tAddr}))