Skip to content

Commit 0d9397a

Browse files
brandonilesBrandon Iles
authored and
aalavandhan
committed
Change burn to burnFrom, and require approval (#47)
* burn -> burnFrom w/ approval * add allowance reduction case * fix warnings Co-authored-by: Brandon Iles <brandon@ampleforth.org>
1 parent a419039 commit 0d9397a

File tree

6 files changed

+157
-27
lines changed

6 files changed

+157
-27
lines changed

‎contracts/_interfaces/IXCAmple.sol

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,4 +5,6 @@ import "uFragments/contracts/interfaces/IAMPL.sol";
55

66
interface IXCAmple is IAMPL {
77
function globalAMPLSupply() external view returns (uint256);
8+
function mint(address who, uint256 xcAmpleAmount) external;
9+
function burnFrom(address who, uint256 xcAmpleAmount) external;
810
}

‎contracts/_mocks/MockXCAmple.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ contract MockXCAmple {
2626
emit Mint(who, value);
2727
}
2828

29-
function burn(address who, uint256 value) external {
29+
function burnFrom(address who, uint256 value) external {
3030
emit Burn(who, value);
3131
}
3232
}

‎contracts/satellite-chain/xc-ampleforth/XCAmple.sol

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -394,14 +394,25 @@ contract XCAmple is IERC20Upgradeable, OwnableUpgradeable {
394394
}
395395

396396
/**
397-
* @dev Burn xcAmples from the beneficiary.
397+
* @dev Destroys `xcAmpleAmount` tokens from `who`, deducting from the caller's
398+
* allowance.
399+
*
400+
* This is only callable by the controller, because we only want to support burn for the
401+
* interchain-transfer case. Otherwise, AMPL on base chain could become locked in the vault.
402+
*
403+
* Requirements:
404+
*
405+
* - the caller must have allowance for ``who``'s tokens of at least
406+
* `xcAmpleAmount`.
398407
*
399408
* @param who The address of the beneficiary.
400409
* @param xcAmpleAmount The amount of xcAmple tokens to be burned.
401410
*/
402-
function burn(address who, uint256 xcAmpleAmount) external onlyController {
411+
function burnFrom(address who, uint256 xcAmpleAmount) external onlyController {
403412
require(who != address(0), "XCAmple: burn address zero address");
404413

414+
_allowedXCAmples[who][msg.sender] = _allowedXCAmples[who][msg.sender].sub(xcAmpleAmount);
415+
405416
uint256 gonValue = xcAmpleAmount.mul(_gonsPerAMPL);
406417
_gonBalances[who] = _gonBalances[who].sub(gonValue);
407418
_totalSupply = _totalSupply.sub(xcAmpleAmount);

‎contracts/satellite-chain/xc-ampleforth/XCAmpleController.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ contract XCAmpleController is OwnableUpgradeable {
126126
* @param xcAmpleAmount The amount of xcAmples to be burnt on this chain.
127127
*/
128128
function burn(address depositor, uint256 xcAmpleAmount) external onlyBridgeGateway {
129-
IXCAmpleSupplyPolicy(xcAmple).burn(depositor, xcAmpleAmount);
129+
IXCAmple(xcAmple).burnFrom(depositor, xcAmpleAmount);
130130
emit GatewayBurn(msg.sender, depositor, xcAmpleAmount);
131131
}
132132

‎test/integration/chain_bridge.js

Lines changed: 89 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -248,10 +248,17 @@ async function execXCSend (
248248
amount,
249249
setApproval = true,
250250
) {
251-
if (setApproval && fromChain === 'base') {
252-
await baseChainAmplContracts.ampl
253-
.connect(fromAccount)
254-
.approve(bridgeContractsMap[fromChain].tokenVault.address, amount);
251+
252+
if (setApproval) {
253+
if (fromChain === 'base') {
254+
await baseChainAmplContracts.ampl
255+
.connect(fromAccount)
256+
.approve(bridgeContractsMap[fromChain].tokenVault.address, amount);
257+
} else {
258+
await amplContractsMap[fromChain].xcAmple
259+
.connect(fromAccount)
260+
.approve(amplContractsMap[fromChain].xcAmpleController.address, amount);
261+
}
255262
}
256263

257264
const policy =
@@ -557,7 +564,7 @@ describe('Transfers scenarios', function () {
557564
await setupContracts();
558565
});
559566

560-
describe('cross-chain transfer when user has NOT approved the vault', function () {
567+
describe('transfer from base when user has NOT approved the vault', function () {
561568
it('should revert', async function () {
562569
await baseChainAmplContracts.ampl
563570
.connect(userBBaseChainWallet)
@@ -578,7 +585,7 @@ describe('Transfers scenarios', function () {
578585
});
579586
});
580587

581-
describe('cross-chain transfer when user has approved the vault', function () {
588+
describe('transfer from base when user has approved the vault', function () {
582589
it('should NOT revert', async function () {
583590
await baseChainAmplContracts.ampl
584591
.connect(userBBaseChainWallet)
@@ -725,6 +732,82 @@ describe('Transfers scenarios', function () {
725732
'0',
726733
);
727734
});
735+
736+
describe('user does not approve Vault on base chain', function() {
737+
it('should revert', async function () {
738+
await checkBalancesAndSupply(
739+
['100000', '100000'],
740+
['0', '0'],
741+
'0',
742+
[],
743+
'0',
744+
);
745+
746+
await baseChainAmplContracts.ampl
747+
.connect(userBBaseChainWallet)
748+
.approve(
749+
bridgeContractsMap['base'].tokenVault.address,
750+
toAmplFixedPt('0'), // Approve 0
751+
);
752+
753+
await expect(
754+
execXCSend(
755+
'base',
756+
'sat1',
757+
userBBaseChainWallet,
758+
userBSatChain1Wallet,
759+
toAmplFixedPt('5000'),
760+
false, // Use existing approval and don't auto-approve.
761+
),
762+
).to.be.reverted;
763+
})
764+
});
765+
766+
describe('user does not approve Controller on satellite chain', function() {
767+
it('should revert', async function () {
768+
769+
// Setup by placing amples on satellite chain
770+
await checkBalancesAndSupply(
771+
['100000', '100000'],
772+
['0', '0'],
773+
'0',
774+
[],
775+
'0',
776+
);
777+
await execXCSend(
778+
'base',
779+
'sat1',
780+
userABaseChainWallet,
781+
userASatChain1Wallet,
782+
toAmplFixedPt('5000'),
783+
);
784+
await checkBalancesAndSupply(
785+
['95000', '100000'],
786+
['5000', '0'],
787+
'5000',
788+
[],
789+
'0',
790+
);
791+
792+
// Test transfer from satellite chain
793+
await amplContractsMap['sat1'].xcAmple
794+
.connect(userASatChain1Wallet)
795+
.approve(
796+
amplContractsMap['sat1'].xcAmpleController.address,
797+
toAmplFixedPt('0'), // Approve 0
798+
);
799+
await expect(
800+
execXCSend(
801+
'sat1',
802+
'base',
803+
userASatChain1Wallet,
804+
userABaseChainWallet,
805+
toAmplFixedPt('1'),
806+
false, // Use existing approval and don't auto-approve.
807+
),
808+
).to.be.reverted;
809+
});
810+
});
728811
});
729812

730813
describe('user transfers from base chain to satellite chain and back around rebase', function () {

‎test/unit/satellite-chain/xc-ampleforth/xc_ample_burn.js

Lines changed: 51 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -30,36 +30,39 @@ async function setupContracts () {
3030
await xcAmple.setController(deployer.getAddress());
3131
}
3232

33-
describe('XCAmple:burn:accessControl', function () {
33+
describe('XCAmple:burnFrom:accessControl', function () {
3434
before('setup XCAmple contract', setupContracts);
3535
beforeEach(async function () {
3636
await xcAmple
3737
.connect(deployer)
3838
.mint(otherUser.getAddress(), unitTokenAmount);
39+
await xcAmple
40+
.connect(otherUser)
41+
.approve(await deployer.getAddress(), unitTokenAmount);
3942
});
4043

4144
it('should NOT be callable by other user', async function () {
4245
await expect(
43-
xcAmple.connect(otherUser).burn(otherUser.getAddress(), unitTokenAmount),
46+
xcAmple.connect(otherUser).burnFrom(otherUser.getAddress(), unitTokenAmount),
4447
).to.be.reverted;
4548
});
4649

47-
it('should be callable by controller', async function () {
50+
it('should be callable by spender', async function () {
4851
await expect(
49-
xcAmple.connect(deployer).burn(otherUser.getAddress(), unitTokenAmount),
52+
xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), unitTokenAmount),
5053
).not.to.be.reverted;
5154
});
5255
});
5356

54-
describe('XCAmple:burn', () => {
57+
describe('XCAmple:burnFrom', () => {
5558
beforeEach('setup XCAmple contract', setupContracts);
5659

5760
describe('when burn address is zero address', () => {
5861
it('should revert', async function () {
5962
await expect(
6063
xcAmple
6164
.connect(deployer)
62-
.burn(ethers.constants.AddressZero, unitTokenAmount),
65+
.burnFrom(ethers.constants.AddressZero, 0),
6366
).to.be.reverted;
6467
});
6568
});
@@ -69,8 +72,21 @@ describe('XCAmple:burn', () => {
6972
const mintAmt = toUFrgDenomination('1000000');
7073
await xcAmple.connect(deployer).mint(otherUser.getAddress(), mintAmt);
7174
const burnAmt = (await xcAmple.balanceOf(otherUser.getAddress())).add(1);
75+
await xcAmple.connect(otherUser).approve(await deployer.getAddress(), burnAmt);
76+
7277
await expect(
73-
xcAmple.connect(deployer).burn(otherUser.getAddress(), burnAmt),
78+
xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), burnAmt),
79+
).to.be.reverted;
80+
});
81+
});
82+
83+
describe('when burn value > approved amount', () => {
84+
it('should revert', async function () {
85+
const mintAmt = toUFrgDenomination('1000000');
86+
await xcAmple.connect(deployer).mint(otherUser.getAddress(), mintAmt);
87+
await xcAmple.connect(otherUser).approve(await deployer.getAddress(), mintAmt.sub(1));
88+
await expect(
89+
xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), mintAmt),
7490
).to.be.reverted;
7591
});
7692
});
@@ -82,23 +98,29 @@ describe('XCAmple:burn', () => {
8298
beforeEach(async function () {
8399
await xcAmple.connect(deployer).mint(deployer.getAddress(), amt2);
84100
await xcAmple.connect(deployer).mint(otherUser.getAddress(), amt1);
101+
await xcAmple.connect(otherUser).approve(deployer.getAddress(), amt1)
85102
});
86103
it('should burn tokens from wallet', async function () {
87-
await xcAmple.connect(deployer).burn(otherUser.getAddress(), amt1);
104+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), amt1);
88105
expect(await xcAmple.balanceOf(otherUser.getAddress())).to.eq(0);
89106
});
90107
it('should NOT affect other wallets', async function () {
91108
expect(await xcAmple.balanceOf(deployer.getAddress())).to.eq(amt2);
92-
await xcAmple.connect(deployer).burn(otherUser.getAddress(), amt1);
109+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), amt1);
93110
expect(await xcAmple.balanceOf(deployer.getAddress())).to.eq(amt2);
94111
});
95112
it('should update the total supply', async function () {
96113
expect(await xcAmple.totalSupply()).to.eq(totalAmt);
97-
await xcAmple.connect(deployer).burn(otherUser.getAddress(), amt1);
114+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), amt1);
98115
expect(await xcAmple.totalSupply()).to.eq(amt2);
99116
});
117+
it('should reduce the approved amount', async function () {
118+
const allowanceBefore = await xcAmple.allowance(otherUser.getAddress(), deployer.getAddress());
119+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), amt1);
120+
expect(await xcAmple.allowance(otherUser.getAddress(), deployer.getAddress())).to.eq(allowanceBefore - amt1);
121+
});
100122
it('should log Transfer to zero address', async function () {
101-
await expect(xcAmple.connect(deployer).burn(otherUser.getAddress(), amt1))
123+
await expect(xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), amt1))
102124
.to.emit(xcAmple, 'Transfer')
103125
.withArgs(
104126
await otherUser.getAddress(),
@@ -115,21 +137,27 @@ describe('XCAmple:burn', () => {
115137

116138
beforeEach(async function () {
117139
await xcAmple.connect(deployer).mint(otherUser.getAddress(), mintAmt);
140+
await xcAmple.connect(otherUser).approve(deployer.getAddress(), mintAmt);
118141
});
119142
it('should burn tokens from wallet', async function () {
120-
await xcAmple.connect(deployer).burn(otherUser.getAddress(), burnAmt);
143+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), burnAmt);
121144
expect(await xcAmple.balanceOf(otherUser.getAddress())).to.eq(
122145
remainingBal,
123146
);
124147
});
125148
it('should update the total supply', async function () {
126149
expect(await xcAmple.totalSupply()).to.eq(mintAmt);
127-
await xcAmple.connect(deployer).burn(otherUser.getAddress(), burnAmt);
150+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), burnAmt);
128151
expect(await xcAmple.totalSupply()).to.eq(remainingBal);
129152
});
153+
it('should reduce the approved amount', async function () {
154+
const allowanceBefore = await xcAmple.allowance(otherUser.getAddress(), deployer.getAddress());
155+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), burnAmt);
156+
expect(await xcAmple.allowance(otherUser.getAddress(), deployer.getAddress())).to.eq(allowanceBefore - burnAmt);
157+
});
130158
it('should log Transfer to zero address', async function () {
131159
await expect(
132-
xcAmple.connect(deployer).burn(otherUser.getAddress(), burnAmt),
160+
xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), burnAmt),
133161
)
134162
.to.emit(xcAmple, 'Transfer')
135163
.withArgs(
@@ -147,22 +175,28 @@ describe('XCAmple:burn', () => {
147175
beforeEach(async function () {
148176
await xcAmple.rebase(1, MAX_SUPPLY);
149177
await xcAmple.connect(deployer).mint(otherUser.getAddress(), MAX_SUPPLY);
178+
await xcAmple.connect(otherUser).approve(deployer.getAddress(), MAX_SUPPLY);
150179
});
151180

152181
it('should burn tokens from wallet', async function () {
153-
await xcAmple.connect(deployer).burn(otherUser.getAddress(), burnAmt);
182+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), burnAmt);
154183
expect(await xcAmple.balanceOf(otherUser.getAddress())).to.eq(
155184
remainingBal,
156185
);
157186
});
158187
it('should update the total supply', async function () {
159188
expect(await xcAmple.totalSupply()).to.eq(MAX_SUPPLY);
160-
await xcAmple.connect(deployer).burn(otherUser.getAddress(), burnAmt);
189+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), burnAmt);
161190
expect(await xcAmple.totalSupply()).to.eq(remainingBal);
162191
});
192+
it('should reduce the approved amount', async function () {
193+
const allowanceBefore = await xcAmple.allowance(otherUser.getAddress(), deployer.getAddress());
194+
await xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), burnAmt);
195+
expect(await xcAmple.allowance(otherUser.getAddress(), deployer.getAddress())).to.eq(allowanceBefore.sub(burnAmt));
196+
});
163197
it('should log Transfer to zero address', async function () {
164198
await expect(
165-
xcAmple.connect(deployer).burn(otherUser.getAddress(), burnAmt),
199+
xcAmple.connect(deployer).burnFrom(otherUser.getAddress(), burnAmt),
166200
)
167201
.to.emit(xcAmple, 'Transfer')
168202
.withArgs(

0 commit comments

Comments
 (0)