From 283b1c7e9f7a7c1a5ba54d60121d173ddcc0e66c Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Thu, 21 Dec 2023 14:58:45 +0100 Subject: [PATCH] Add burnFrom for compatibility with CCIP and bridge, rename forceBurn in burn --- contracts/interfaces/ICCIPToken.sol | 4 +- contracts/modules/CMTAT_BASE.sol | 4 +- .../modules/security/AuthorizationModule.sol | 1 + .../modules/wrapper/core/ERC20BaseModule.sol | 1 + .../modules/wrapper/core/ERC20BurnModule.sol | 61 ++++++--- .../extensions/DebtModule/DebtBaseModule.sol | 54 ++++---- test/common/ERC20BurnModuleCommon.js | 126 +++++------------- ...C20SnapshotModuleOnePlannedSnapshotTest.js | 2 +- test/utils.js | 2 + 9 files changed, 111 insertions(+), 144 deletions(-) diff --git a/contracts/interfaces/ICCIPToken.sol b/contracts/interfaces/ICCIPToken.sol index 22d5e7b6..4ab3564e 100644 --- a/contracts/interfaces/ICCIPToken.sol +++ b/contracts/interfaces/ICCIPToken.sol @@ -10,11 +10,11 @@ interface ICCIPMintERC20 { function mint(address account, uint256 value) external; } -interface ICCIPBurnERC20 { +interface ICCIPBurnFromERC20 { /// @notice Burns tokens from a given address.. /// @param account The address to burn tokens from. /// @param value The number of tokens to be burned. /// @dev this function decreases the total supply. - function burn(address account, uint256 value) external; + function burnFrom(address account, uint256 value) external; } \ No newline at end of file diff --git a/contracts/modules/CMTAT_BASE.sol b/contracts/modules/CMTAT_BASE.sol index 4068120e..64dd0eb9 100644 --- a/contracts/modules/CMTAT_BASE.sol +++ b/contracts/modules/CMTAT_BASE.sol @@ -182,12 +182,12 @@ abstract contract CMTAT_BASE is @param amountToBurn number of tokens to burn @param amountToMint number of tokens to mint @dev - - The access control is managed by the functions burn and mint + - The access control is managed by the functions burn (ERC20BurnModule) and mint (ERC20MintModule) - Input validation is also managed by the functions burn and mint - You can mint more tokens than burnt */ function burnAndMint(address from, address to, uint256 amountToBurn, uint256 amountToMint, string calldata reason) public { - burnWithReason(from, amountToBurn, reason); + burn(from, amountToBurn, reason); mint(to, amountToMint); } diff --git a/contracts/modules/security/AuthorizationModule.sol b/contracts/modules/security/AuthorizationModule.sol index e2d718a3..06d86a88 100644 --- a/contracts/modules/security/AuthorizationModule.sol +++ b/contracts/modules/security/AuthorizationModule.sol @@ -15,6 +15,7 @@ abstract contract AuthorizationModule is AccessControlUpgradeable { event AuthorizationEngine(IAuthorizationEngine indexed newAuthorizationEngine); // BurnModule bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE"); + bytes32 public constant BURNER_FROM_ROLE = keccak256("BURNER_FROM_ROLE"); // CreditEvents bytes32 public constant DEBT_CREDIT_EVENT_ROLE = keccak256("DEBT_CREDIT_EVENT_ROLE"); diff --git a/contracts/modules/wrapper/core/ERC20BaseModule.sol b/contracts/modules/wrapper/core/ERC20BaseModule.sol index 43743437..0a2af91d 100644 --- a/contracts/modules/wrapper/core/ERC20BaseModule.sol +++ b/contracts/modules/wrapper/core/ERC20BaseModule.sol @@ -11,6 +11,7 @@ abstract contract ERC20BaseModule is ERC20Upgradeable { /* Events */ /** * @notice Emitted when the specified `spender` spends the specified `value` tokens owned by the specified `owner` reducing the corresponding allowance. + * @dev The allowance can be also "spend" with the function BurnFrom, but in this case, the emitted event is BurnFrom. */ event Spend(address indexed owner, address indexed spender, uint256 value); diff --git a/contracts/modules/wrapper/core/ERC20BurnModule.sol b/contracts/modules/wrapper/core/ERC20BurnModule.sol index 07178f4a..43467e71 100644 --- a/contracts/modules/wrapper/core/ERC20BurnModule.sol +++ b/contracts/modules/wrapper/core/ERC20BurnModule.sol @@ -6,12 +6,15 @@ import "../../../../openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC import "../../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; import "../../security/AuthorizationModule.sol"; import "../../../interfaces/ICCIPToken.sol"; -abstract contract ERC20BurnModule is ERC20Upgradeable, ICCIPBurnERC20, AuthorizationModule { +abstract contract ERC20BurnModule is ERC20Upgradeable, ICCIPBurnFromERC20, AuthorizationModule { /** * @notice Emitted when the specified `value` amount of tokens owned by `owner`are destroyed with the given `reason` */ event Burn(address indexed owner, uint256 value, string reason); - + /** + * @notice Emitted when the specified `spender` burns the specified `value` tokens owned by the specified `owner` reducing the corresponding allowance. + */ + event BurnFrom(address indexed owner, address indexed spender, uint256 value); function __ERC20BurnModule_init_unchained() internal onlyInitializing { // no variable to initialize } @@ -25,7 +28,7 @@ abstract contract ERC20BurnModule is ERC20Upgradeable, ICCIPBurnERC20, Authoriza * Requirements: * - the caller must have the `BURNER_ROLE`. */ - function burnWithReason( + function burn( address account, uint256 value, string calldata reason @@ -34,21 +37,6 @@ abstract contract ERC20BurnModule is ERC20Upgradeable, ICCIPBurnERC20, Authoriza emit Burn(account, value, reason); } - /** - * @notice burn with empty string as reason - * @param account token holder - * @param value amount of tokens - * @dev - * used to be compatible with CCIP pool system - */ - function burn( - address account, - uint256 value - ) public onlyRole(BURNER_ROLE) { - _burn(account, value); - emit Burn(account, value, ""); - } - /** * @@ -87,5 +75,42 @@ abstract contract ERC20BurnModule is ERC20Upgradeable, ICCIPBurnERC20, Authoriza } } + /** + * @notice Destroys `amount` tokens from `account`, deducting from the caller's + * allowance. + * @dev + * Can be used to authorize a bridge (e.g. CCIP) to burn token owned by the bridge + * No string parameter reason to be compatible with Bridge, e.g. CCIP + * + * See {ERC20-_burn} and {ERC20-allowance}. + * + * Requirements: + * + * - the caller must have allowance for ``accounts``'s tokens of at least + * `value`. + */ + function burnFrom(address account, uint256 value) + public + onlyRole(BURNER_FROM_ROLE) + { + // Allowance check + address sender = _msgSender(); + uint256 currentAllowance = allowance(account, sender); + if(currentAllowance < value){ + // ERC-6093 + revert ERC20InsufficientAllowance(sender, currentAllowance, value); + } + // Update allowance + unchecked { + _approve(account, sender, currentAllowance - value); + } + // burn + _burn(account, value); + // We also emit a burn event since its a burn operation + emit Burn(account, value, "burnFrom"); + // Specific event for the operation + emit BurnFrom(account, sender, value); + } + uint256[50] private __gap; } diff --git a/contracts/modules/wrapper/extensions/DebtModule/DebtBaseModule.sol b/contracts/modules/wrapper/extensions/DebtModule/DebtBaseModule.sol index 1fbfd8d8..9193531a 100644 --- a/contracts/modules/wrapper/extensions/DebtModule/DebtBaseModule.sol +++ b/contracts/modules/wrapper/extensions/DebtModule/DebtBaseModule.sol @@ -59,9 +59,9 @@ abstract contract DebtBaseModule is // no variable to initialize } - /* - @notice Set all attributes of debt - The values of all attributes will be changed even if the new values are the same as the current ones + /** + * @notice Set all attributes of debt + * The values of all attributes will be changed even if the new values are the same as the current ones */ function setDebt(DebtBase calldata debt_) public onlyRole(DEBT_ROLE) { debt = debt_; @@ -96,8 +96,8 @@ abstract contract DebtBaseModule is emit CouponFrequency(debt_.couponFrequency, debt_.couponFrequency); } - /* - @notice The call will be reverted if the new value of interestRate is the same as the current one + /** + * @notice The call will be reverted if the new value of interestRate is the same as the current one */ function setInterestRate(uint256 interestRate_) public onlyRole(DEBT_ROLE) { if (interestRate_ == debt.interestRate) { @@ -107,8 +107,8 @@ abstract contract DebtBaseModule is emit InterestRate(interestRate_); } - /* - @notice The call will be reverted if the new value of parValue is the same as the current one + /** + * @notice The call will be reverted if the new value of parValue is the same as the current one */ function setParValue(uint256 parValue_) public onlyRole(DEBT_ROLE) { if (parValue_ == debt.parValue) { @@ -118,8 +118,8 @@ abstract contract DebtBaseModule is emit ParValue(parValue_); } - /* - @notice The Guarantor will be changed even if the new value is the same as the current one + /** + * @notice The Guarantor will be changed even if the new value is the same as the current one */ function setGuarantor( string calldata guarantor_ @@ -128,8 +128,8 @@ abstract contract DebtBaseModule is emit Guarantor(guarantor_, guarantor_); } - /* - @notice The bonHolder will be changed even if the new value is the same as the current one + /** + * @notice The bonHolder will be changed even if the new value is the same as the current one */ function setBondHolder( string calldata bondHolder_ @@ -138,8 +138,8 @@ abstract contract DebtBaseModule is emit BondHolder(bondHolder_, bondHolder_); } - /* - @notice The maturityDate will be changed even if the new value is the same as the current one + /** + * @notice The maturityDate will be changed even if the new value is the same as the current one */ function setMaturityDate( string calldata maturityDate_ @@ -148,8 +148,8 @@ abstract contract DebtBaseModule is emit MaturityDate(maturityDate_, maturityDate_); } - /* - @notice The interestScheduleFormat will be changed even if the new value is the same as the current one + /** + * @notice The interestScheduleFormat will be changed even if the new value is the same as the current one */ function setInterestScheduleFormat( string calldata interestScheduleFormat_ @@ -161,8 +161,8 @@ abstract contract DebtBaseModule is ); } - /* - @notice The interestPaymentDate will be changed even if the new value is the same as the current one + /** + * @notice The interestPaymentDate will be changed even if the new value is the same as the current one */ function setInterestPaymentDate( string calldata interestPaymentDate_ @@ -171,8 +171,8 @@ abstract contract DebtBaseModule is emit InterestPaymentDate(interestPaymentDate_, interestPaymentDate_); } - /* - @notice The dayCountConvention will be changed even if the new value is the same as the current one + /** + * @notice The dayCountConvention will be changed even if the new value is the same as the current one */ function setDayCountConvention( string calldata dayCountConvention_ @@ -181,8 +181,8 @@ abstract contract DebtBaseModule is emit DayCountConvention(dayCountConvention_, dayCountConvention_); } - /* - @notice The businessDayConvention will be changed even if the new value is the same as the current one + /** + * @notice The businessDayConvention will be changed even if the new value is the same as the current one */ function setBusinessDayConvention( string calldata businessDayConvention_ @@ -194,8 +194,8 @@ abstract contract DebtBaseModule is ); } - /* - @notice The publicHolidayCalendar will be changed even if the new value is the same as the current one + /** + * @notice The publicHolidayCalendar will be changed even if the new value is the same as the current one */ function setPublicHolidaysCalendar( string calldata publicHolidaysCalendar_ @@ -207,8 +207,8 @@ abstract contract DebtBaseModule is ); } - /* - @notice The issuanceDate will be changed even if the new value is the same as the current one + /** + * @notice The issuanceDate will be changed even if the new value is the same as the current one */ function setIssuanceDate( string calldata issuanceDate_ @@ -217,8 +217,8 @@ abstract contract DebtBaseModule is emit IssuanceDate(issuanceDate_, issuanceDate_); } - /* - @notice The couponFrequency will be changed even if the new value is the same as the current one + /** + * @notice The couponFrequency will be changed even if the new value is the same as the current one */ function setCouponFrequency( string calldata couponFrequency_ diff --git a/test/common/ERC20BurnModuleCommon.js b/test/common/ERC20BurnModuleCommon.js index 63d07283..b88c7822 100644 --- a/test/common/ERC20BurnModuleCommon.js +++ b/test/common/ERC20BurnModuleCommon.js @@ -1,12 +1,12 @@ const { BN, expectEvent } = require('@openzeppelin/test-helpers') -const { BURNER_ROLE, ZERO_ADDRESS } = require('../utils') +const { BURNER_ROLE, BURNER_FROM_ROLE, ZERO_ADDRESS } = require('../utils') const { expectRevertCustomError } = require('../../openzeppelin-contracts-upgradeable/test/helpers/customError.js') const { should } = require('chai').should() function ERC20BurnModuleCommon (admin, address1, address2) { - context('BurnWithReason', function () { + context('burn', function () { const INITIAL_SUPPLY = new BN(50) const REASON = 'BURN_TEST' const VALUE1 = new BN(20) @@ -22,7 +22,7 @@ function ERC20BurnModuleCommon (admin, address1, address2) { it('testCanBeBurntByAdmin', async function () { // Act // Burn 20 - this.logs = await this.cmtat.burnWithReason(address1, VALUE1, REASON, { + this.logs = await this.cmtat.burn(address1, VALUE1, REASON, { from: admin }) // Assert @@ -46,7 +46,7 @@ function ERC20BurnModuleCommon (admin, address1, address2) { // Burn 30 // Act - this.logs = await this.cmtat.burnWithReason(address1, DIFFERENCE, REASON, { + this.logs = await this.cmtat.burn(address1, DIFFERENCE, REASON, { from: admin }) @@ -72,7 +72,7 @@ function ERC20BurnModuleCommon (admin, address1, address2) { // Arrange await this.cmtat.grantRole(BURNER_ROLE, address2, { from: admin }) // Act - this.logs = await this.cmtat.burnWithReason(address1, VALUE1, REASON, { + this.logs = await this.cmtat.burn(address1, VALUE1, REASON, { from: address2 }); // Assert @@ -102,7 +102,7 @@ function ERC20BurnModuleCommon (admin, address1, address2) { const ADDRESS1_BALANCE = await this.cmtat.balanceOf(address1) // Act await expectRevertCustomError( - this.cmtat.burnWithReason(address1, AMOUNT_TO_BURN, '', { from: admin }), + this.cmtat.burn(address1, AMOUNT_TO_BURN, '', { from: admin }), 'ERC20InsufficientBalance', [address1, ADDRESS1_BALANCE, AMOUNT_TO_BURN] ) @@ -110,18 +110,16 @@ function ERC20BurnModuleCommon (admin, address1, address2) { it('testCannotBeBurntWithoutBurnerRole', async function () { await expectRevertCustomError( - this.cmtat.burnWithReason(address1, 20, '', { from: address2 }), + this.cmtat.burn(address1, 20, '', { from: address2 }), 'AccessControlUnauthorizedAccount', [address2, BURNER_ROLE] ) }) }) - context('BurnWithEmptyReason', function () { + context('burnFrom', function () { const INITIAL_SUPPLY = new BN(50) - const REASON = 'BURN_TEST' const VALUE1 = new BN(20) - const DIFFERENCE = INITIAL_SUPPLY.sub(VALUE1) beforeEach(async function () { await this.cmtat.mint(address1, INITIAL_SUPPLY, { from: admin }); @@ -130,105 +128,45 @@ function ERC20BurnModuleCommon (admin, address1, address2) { ) }) - it('testCanBeBurntByAdmin', async function () { - // Act - // Burn 20 - this.logs = await this.cmtat.burn(address1, VALUE1, { - from: admin - }) - // Assert - // emits a Transfer event - expectEvent(this.logs, 'Transfer', { - from: address1, - to: ZERO_ADDRESS, - value: VALUE1 - }) - // Emits a Burn event - expectEvent(this.logs, 'Burn', { - owner: address1, - value: VALUE1, - reason: '' - }); - // Check balances and total supply - (await this.cmtat.balanceOf(address1)).should.be.bignumber.equal( - DIFFERENCE - ); - (await this.cmtat.totalSupply()).should.be.bignumber.equal(DIFFERENCE) - - // Burn 30 - // Act - this.logs = await this.cmtat.burn(address1, DIFFERENCE, { - from: admin - }) - - // Assert - // Emits a Transfer event - expectEvent(this.logs, 'Transfer', { - from: address1, - to: ZERO_ADDRESS, - value: DIFFERENCE - }) - // Emits a Burn event - expectEvent(this.logs, 'Burn', { - owner: address1, - value: DIFFERENCE, - reason: '' - }); - // Check balances and total supply - (await this.cmtat.balanceOf(address1)).should.be.bignumber.equal(BN(0)); - (await this.cmtat.totalSupply()).should.be.bignumber.equal(BN(0)) - }) - - it('testCanBeBurntByBurnerRole', async function () { + it('canBeBurnFrom', async function () { // Arrange - await this.cmtat.grantRole(BURNER_ROLE, address2, { from: admin }) + const AMOUNT_TO_BURN = BN(20) + await this.cmtat.grantRole(BURNER_FROM_ROLE, address2, { from: admin }) + await this.cmtat.approve(address2, 50, { from: address1 }) // Act - this.logs = await this.cmtat.burn(address1, VALUE1, { - from: address2 - }); + this.logs = await this.cmtat.burnFrom(address1, AMOUNT_TO_BURN, { from: address2 }) // Assert - (await this.cmtat.balanceOf(address1)).should.be.bignumber.equal( - DIFFERENCE - ); - (await this.cmtat.totalSupply()).should.be.bignumber.equal(DIFFERENCE) - - // Emits a Transfer event expectEvent(this.logs, 'Transfer', { from: address1, to: ZERO_ADDRESS, - value: VALUE1 + value: AMOUNT_TO_BURN }) - - // Emits a Burn event - expectEvent(this.logs, 'Burn', { + expectEvent(this.logs, 'BurnFrom', { owner: address1, - value: VALUE1, - reason: '' - }) + spender: address2, + value: AMOUNT_TO_BURN + }); + (await this.cmtat.balanceOf(address1)).should.be.bignumber.equal('30'); + (await this.cmtat.totalSupply()).should.be.bignumber.equal('30') }) - it('testCannotBeBurntIfBalanceExceeds', async function () { - // error AccessControlUnauthorizedAccount(address account, bytes32 neededRole); - const AMOUNT_TO_BURN = BN(200) - const ADDRESS1_BALANCE = await this.cmtat.balanceOf(address1) - // Act + it('TestCannotBeBurnWithoutAllowance', async function () { + const AMOUNT_TO_BURN = 20 await expectRevertCustomError( - this.cmtat.burn(address1, AMOUNT_TO_BURN, { from: admin }), - 'ERC20InsufficientBalance', - [address1, ADDRESS1_BALANCE, AMOUNT_TO_BURN] - ) + this.cmtat.burnFrom(address1, AMOUNT_TO_BURN, { from: admin }), + 'ERC20InsufficientAllowance', + [admin, 0, AMOUNT_TO_BURN]) }) - it('testCannotBeBurntWithoutBurnerRole', async function () { + it('testCannotBeBurntWithoutBurnerFromRole', async function () { await expectRevertCustomError( - this.cmtat.burn(address1, 20, { from: address2 }), + this.cmtat.burnFrom(address1, 20, { from: address2 }), 'AccessControlUnauthorizedAccount', - [address2, BURNER_ROLE] - ) + [address2, BURNER_FROM_ROLE]) }) }) - context('BurnBatch', function () { + context('burnBatch', function () { const REASON = 'BURN_TEST' const TOKEN_HOLDER = [admin, address1, address2] const TOKEN_SUPPLY_BY_HOLDERS = [BN(10), BN(100), BN(1000)] @@ -380,7 +318,7 @@ function ERC20BurnModuleCommon (admin, address1, address2) { ) }) - it('testCannotBurnBatchIfLengthMismatchMissingAddresses', async function () { + it('testCannotburnBatchIfLengthMismatchMissingAddresses', async function () { // Number of addresses is insufficient const TOKEN_HOLDER_INVALID = [admin, address1] await expectRevertCustomError( @@ -395,7 +333,7 @@ function ERC20BurnModuleCommon (admin, address1, address2) { ) }) - it('testCannotBurnBatchIfLengthMismatchTooManyAddresses', async function () { + it('testCannotburnBatchIfLengthMismatchTooManyAddresses', async function () { // There are too many addresses const TOKEN_HOLDER_INVALID = [admin, address1, address1, address1] await expectRevertCustomError( @@ -410,7 +348,7 @@ function ERC20BurnModuleCommon (admin, address1, address2) { ) }) - it('testCannotBurnBatchIfAccountsIsEmpty', async function () { + it('testCannotburnBatchIfAccountsIsEmpty', async function () { const TOKEN_ADDRESS_TOS_INVALID = [] await expectRevertCustomError( this.cmtat.burnBatch( diff --git a/test/common/ERC20SnapshotModuleCommon/global/ERC20SnapshotModuleOnePlannedSnapshotTest.js b/test/common/ERC20SnapshotModuleCommon/global/ERC20SnapshotModuleOnePlannedSnapshotTest.js index 8f8adb6f..989f26d9 100644 --- a/test/common/ERC20SnapshotModuleCommon/global/ERC20SnapshotModuleOnePlannedSnapshotTest.js +++ b/test/common/ERC20SnapshotModuleCommon/global/ERC20SnapshotModuleOnePlannedSnapshotTest.js @@ -86,7 +86,7 @@ function ERC20SnapshotModuleOnePlannedSnapshotTest ( ) // Act - await this.cmtat.burnWithReason(address1, BURN_AMOUNT, reason, { + await this.cmtat.burn(address1, BURN_AMOUNT, reason, { from: admin, gas: 5000000, gasPrice: 500000000 diff --git a/test/utils.js b/test/utils.js index a09b6992..ed8baa25 100644 --- a/test/utils.js +++ b/test/utils.js @@ -3,6 +3,8 @@ module.exports = { '0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6', // keccak256("MINTER_ROLE"); BURNER_ROLE: '0x3c11d16cbaffd01df69ce1c404f6340ee057498f5f00246190ea54220576a848', // keccak256("BURNER_ROLE"); + BURNER_FROM_ROLE: + '0x5bfe08abba057c54e6a28bce27ce8c53eb21d7a94376a70d475b5dee60b6c4e2', // keccak256("BURNER_FROM_ROLE"); ENFORCER_ROLE: '0x973ef39d76cc2c6090feab1c030bec6ab5db557f64df047a4c4f9b5953cf1df3', // keccak256("ENFORCER_ROLE"); PAUSER_ROLE: