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

Auto 9427 withdraw erc 20 fees does not check reserve amount #12582

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Auto 9427 withdraw erc 20 fees does not check reserve amount #12582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9275e3b
write failing test; reorder event
RyanRHall Mar 25, 2024
76460e9
fix withdrawERC20Fees bug
RyanRHall Mar 25, 2024
8f7ea0a
regenerate wrappers; add changeset
RyanRHall Mar 25, 2024
b546312
fix test
RyanRHall Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/quick-berries-sin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

fix bug in auto2.3 withdrawERC20Fees
5 changes: 5 additions & 0 deletions contracts/.changeset/early-hairs-wonder.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@chainlink/contracts": patch
---

fix bug in auto2.3 withdrawERC20Fees
10 changes: 5 additions & 5 deletions contracts/src/v0.8/automation/dev/interfaces/v2_3/IAutomationRegistryMaster2_3.sol
View file
Open in desktop

Large diffs are not rendered by default.

83 changes: 51 additions & 32 deletions contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ contract SetUp is BaseTest {
"",
""
);

vm.startPrank(OWNER);
registry.addFunds(linkUpkeepID, registry.getMinBalanceForUpkeep(linkUpkeepID));
registry.addFunds(usdUpkeepID, registry.getMinBalanceForUpkeep(usdUpkeepID));
registry.addFunds(nativeUpkeepID, registry.getMinBalanceForUpkeep(nativeUpkeepID));
vm.stopPrank();
}
}

Expand Down Expand Up @@ -140,32 +146,34 @@ contract AddFunds is SetUp {
}

function test_anyoneCanAddFunds() public {
assertEq(registry.getBalance(linkUpkeepID), 0);
uint256 startAmount = registry.getBalance(linkUpkeepID);
vm.prank(UPKEEP_ADMIN);
registry.addFunds(linkUpkeepID, 1);
assertEq(registry.getBalance(linkUpkeepID), 1);
assertEq(registry.getBalance(linkUpkeepID), startAmount + 1);
vm.prank(STRANGER);
registry.addFunds(linkUpkeepID, 1);
assertEq(registry.getBalance(linkUpkeepID), 2);
assertEq(registry.getBalance(linkUpkeepID), startAmount + 2);
}

function test_movesFundFromCorrectToken() public {
vm.startPrank(UPKEEP_ADMIN);

uint256 startBalanceLINK = linkToken.balanceOf(address(registry));
uint256 startBalanceUSDToken = usdToken.balanceOf(address(registry));
uint256 startLinkUpkeepBalance = registry.getBalance(linkUpkeepID);
uint256 startUSDUpkeepBalance = registry.getBalance(usdUpkeepID);

registry.addFunds(linkUpkeepID, 1);
assertEq(registry.getBalance(linkUpkeepID), 1);
assertEq(registry.getBalance(usdUpkeepID), 0);
assertEq(linkToken.balanceOf(address(registry)), startBalanceLINK + 1);
assertEq(usdToken.balanceOf(address(registry)), startBalanceUSDToken);
assertEq(registry.getBalance(linkUpkeepID), startBalanceLINK + 1);
assertEq(registry.getBalance(usdUpkeepID), startBalanceUSDToken);
assertEq(linkToken.balanceOf(address(registry)), startLinkUpkeepBalance + 1);
assertEq(usdToken.balanceOf(address(registry)), startUSDUpkeepBalance);

registry.addFunds(usdUpkeepID, 2);
assertEq(registry.getBalance(linkUpkeepID), 1);
assertEq(registry.getBalance(usdUpkeepID), 2);
assertEq(linkToken.balanceOf(address(registry)), startBalanceLINK + 1);
assertEq(usdToken.balanceOf(address(registry)), startBalanceUSDToken + 2);
assertEq(registry.getBalance(linkUpkeepID), startBalanceLINK + 1);
assertEq(registry.getBalance(usdUpkeepID), startBalanceUSDToken + 2);
assertEq(linkToken.balanceOf(address(registry)), startLinkUpkeepBalance + 1);
assertEq(usdToken.balanceOf(address(registry)), startUSDUpkeepBalance + 2);
}

function test_emitsAnEvent() public {
Expand All @@ -177,78 +185,89 @@ contract AddFunds is SetUp {
}

contract Withdraw is SetUp {
address internal aMockAddress = address(0x1111111111111111111111111111111111111113);
address internal aMockAddress = randomAddress();

function testLinkAvailableForPaymentReturnsLinkBalance() public {
uint256 startBalance = linkToken.balanceOf(address(registry));
int256 startLinkAvailable = registry.linkAvailableForPayment();

//simulate a deposit of link to the liquidity pool
_mintLink(address(registry), 1e10);

//check there's a balance
assertGt(linkToken.balanceOf(address(registry)), 0);
assertEq(linkToken.balanceOf(address(registry)), startBalance + 1e10);

//check the link available for payment is the link balance
assertEq(uint256(registry.linkAvailableForPayment()), linkToken.balanceOf(address(registry)));
//check the link available has increased by the same amount
assertEq(uint256(registry.linkAvailableForPayment()), uint256(startLinkAvailable) + 1e10);
}

function testWithdrawLinkFeesRevertsBecauseOnlyFinanceAdminAllowed() public {
function testWithdrawLinkRevertsBecauseOnlyFinanceAdminAllowed() public {
vm.expectRevert(abi.encodeWithSelector(Registry.OnlyFinanceAdmin.selector));
registry.withdrawLinkFees(aMockAddress, 1);
registry.withdrawLink(aMockAddress, 1);
}

function testWithdrawLinkFeesRevertsBecauseOfInsufficientBalance() public {
function testWithdrawLinkRevertsBecauseOfInsufficientBalance() public {
vm.startPrank(FINANCE_ADMIN);

// try to withdraw 1 link while there is 0 balance
vm.expectRevert(abi.encodeWithSelector(Registry.InsufficientBalance.selector, 0, 1));
registry.withdrawLinkFees(aMockAddress, 1);
registry.withdrawLink(aMockAddress, 1);

vm.stopPrank();
}

function testWithdrawLinkFeesRevertsBecauseOfInvalidRecipient() public {
function testWithdrawLinkRevertsBecauseOfInvalidRecipient() public {
vm.startPrank(FINANCE_ADMIN);

// try to withdraw 1 link while there is 0 balance
vm.expectRevert(abi.encodeWithSelector(Registry.InvalidRecipient.selector));
registry.withdrawLinkFees(ZERO_ADDRESS, 1);
registry.withdrawLink(ZERO_ADDRESS, 1);

vm.stopPrank();
}

function testWithdrawLinkFeeSuccess() public {
function testWithdrawLinkSuccess() public {
//simulate a deposit of link to the liquidity pool
_mintLink(address(registry), 1e10);

//check there's a balance
assertGt(linkToken.balanceOf(address(registry)), 0);
uint256 startBalance = linkToken.balanceOf(address(registry));

vm.startPrank(FINANCE_ADMIN);

// try to withdraw 1 link while there is a ton of link available
registry.withdrawLinkFees(aMockAddress, 1);
registry.withdrawLink(aMockAddress, 1);

vm.stopPrank();

assertEq(linkToken.balanceOf(address(aMockAddress)), 1);
assertEq(linkToken.balanceOf(address(registry)), 1e10 - 1);
assertEq(linkToken.balanceOf(address(registry)), startBalance - 1);
}

function test_WithdrawERC20Fees_RespectsReserveAmount() public {
assertEq(registry.getBalance(usdUpkeepID), registry.getReserveAmount(address(usdToken)));
vm.startPrank(FINANCE_ADMIN);
vm.expectRevert(abi.encodeWithSelector(Registry.InsufficientBalance.selector, 0, 1));
registry.withdrawERC20Fees(address(usdToken), FINANCE_ADMIN, 1);
}

function testWithdrawERC20FeeSuccess() public {
// simulate a deposit of ERC20 to the liquidity pool
// deposit excess USDToken to the registry (this goes to the "finance withdrawable" pool be default)
uint256 startReserveAmount = registry.getReserveAmount(address(usdToken));
uint256 startAmount = usdToken.balanceOf(address(registry));
_mintERC20(address(registry), 1e10);

// check there's a balance
assertGt(usdToken.balanceOf(address(registry)), 0);
// depositing shouldn't change reserve amount
assertEq(registry.getReserveAmount(address(usdToken)), startReserveAmount);

vm.startPrank(FINANCE_ADMIN);

// try to withdraw 1 link while there is a ton of link available
// try to withdraw 1 USDToken
registry.withdrawERC20Fees(address(usdToken), aMockAddress, 1);

vm.stopPrank();

assertEq(usdToken.balanceOf(address(aMockAddress)), 1);
assertEq(usdToken.balanceOf(address(registry)), 1e10 - 1);
assertEq(usdToken.balanceOf(address(registry)), startAmount + 1e10 - 1);
assertEq(registry.getReserveAmount(address(usdToken)), startReserveAmount);
}
}

Expand Down
4 changes: 2 additions & 2 deletions contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryBase2_3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
error IncorrectNumberOfSignatures();
error IncorrectNumberOfSigners();
error IndexOutOfRange();
error InsufficientBalance(int256 available, uint256 requested);
error InsufficientBalance(uint256 available, uint256 requested);
error InvalidBillingToken();
error InvalidDataLength();
error InvalidFeed();
Expand Down Expand Up @@ -483,7 +483,7 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
event Unpaused(address account);
// Event to emit when a billing configuration is set
event BillingConfigSet(IERC20 indexed token, BillingConfig config);
event FeesWithdrawn(address indexed recipient, address indexed assetAddress, uint256 amount);
event FeesWithdrawn(address indexed assetAddress, address indexed recipient, uint256 amount);

/**
* @param link address of the LINK Token
Expand Down
19 changes: 12 additions & 7 deletions contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryLogicB2_3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,27 +212,32 @@ contract AutomationRegistryLogicB2_3 is AutomationRegistryBase2_3, Chainable {
return int256(i_link.balanceOf(address(this))) - int256(s_reserveAmounts[IERC20(address(i_link))]);
}

function withdrawLinkFees(address to, uint256 amount) external {
function withdrawLink(address to, uint256 amount) external {
_onlyFinanceAdminAllowed();
if (to == ZERO_ADDRESS) revert InvalidRecipient();

int256 available = linkAvailableForPayment();
if (available < 0 || amount > uint256(available)) revert InsufficientBalance(available, amount);
if (available < 0) {
revert InsufficientBalance(0, amount);
} else if (amount > uint256(available)) {
revert InsufficientBalance(uint256(available), amount);
}

bool transferStatus = i_link.transfer(to, amount);
if (!transferStatus) {
revert TransferFailed();
}
emit FeesWithdrawn(to, address(i_link), amount);
emit FeesWithdrawn(address(i_link), to, amount);
}

function withdrawERC20Fees(address assetAddress, address to, uint256 amount) external {
function withdrawERC20Fees(IERC20 asset, address to, uint256 amount) external {
_onlyFinanceAdminAllowed();
if (to == ZERO_ADDRESS) revert InvalidRecipient();
uint256 available = asset.balanceOf(address(this)) - s_reserveAmounts[asset];
if (amount > available) revert InsufficientBalance(available, amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


IERC20(assetAddress).safeTransfer(to, amount);

emit FeesWithdrawn(to, assetAddress, amount);
asset.safeTransfer(to, amount);
emit FeesWithdrawn(address(asset), to, amount);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions contracts/test/v0.8/automation/AutomationRegistry2_3.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4636,7 +4636,7 @@
describe('#withdrawOwnerFunds', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we update this test name?

it('can only be called by finance admin', async () => {
await evmRevert(
registry.connect(keeper1).withdrawLinkFees(zeroAddress, 1),
registry.connect(keeper1).withdrawLink(zeroAddress, 1),
'OnlyFinanceAdmin()',
)
})
Expand Down Expand Up @@ -4693,7 +4693,7 @@
// Now withdraw
await registry
.connect(financeAdmin)
.withdrawLinkFees(await owner.getAddress(), ownerRegistryBalance)
.withdrawLink(await owner.getAddress(), ownerRegistryBalance)

ownerRegistryBalance = await registry.linkAvailableForPayment()
const ownerAfter = await linkToken.balanceOf(await owner.getAddress())
Expand Down Expand Up @@ -5218,8 +5218,8 @@

describe('when called by the owner when the admin has just canceled', () => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
// @ts-ignore

Check warning on line 5221 in contracts/test/v0.8/automation/AutomationRegistry2_3.test.ts

View workflow job for this annotation

GitHub Actions / Solidity Lint 

Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
let oldExpiration: BigNumber

Check warning on line 5222 in contracts/test/v0.8/automation/AutomationRegistry2_3.test.ts

View workflow job for this annotation

GitHub Actions / Solidity Lint 

'oldExpiration' is assigned a value but never used. Allowed unused vars must match /^_/u

beforeEach(async () => {
await registry.connect(admin).cancelUpkeep(upkeepId)
Expand Down
32 changes: 16 additions & 16 deletions ...erated/automation_registry_logic_a_wrapper_2_3/automation_registry_logic_a_wrapper_2_3.go
View file
Open in desktop

Large diffs are not rendered by default.

62 changes: 31 additions & 31 deletions ...erated/automation_registry_logic_b_wrapper_2_3/automation_registry_logic_b_wrapper_2_3.go
View file
Open in desktop

Large diffs are not rendered by default.

32 changes: 16 additions & 16 deletions ...gethwrappers/generated/automation_registry_wrapper_2_3/automation_registry_wrapper_2_3.go
View file
Open in desktop

Large diffs are not rendered by default.

60 changes: 30 additions & 30 deletions ...ated/i_automation_registry_master_wrapper_2_3/i_automation_registry_master_wrapper_2_3.go
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions core/gethwrappers/generation/generated-wrapper-dependency-versions-do-not-edit.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ automation_forwarder_logic: ../../contracts/solc/v0.8.16/AutomationForwarderLogi
automation_registrar_wrapper2_1: ../../contracts/solc/v0.8.16/AutomationRegistrar2_1/AutomationRegistrar2_1.abi ../../contracts/solc/v0.8.16/AutomationRegistrar2_1/AutomationRegistrar2_1.bin eb06d853aab39d3196c593b03e555851cbe8386e0fe54a74c2479f62d14b3c42
automation_registrar_wrapper2_3: ../../contracts/solc/v0.8.19/AutomationRegistrar2_3/AutomationRegistrar2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistrar2_3/AutomationRegistrar2_3.bin 20fac1208261e866caa1f3ffc71030f682a96761bebe79e5ecd71186fce86c60
automation_registry_logic_a_wrapper_2_2: ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_2/AutomationRegistryLogicA2_2.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_2/AutomationRegistryLogicA2_2.bin 2f267fb8467a15c587ce4586ac56069f7229344ad3936430d7c7624c0528a171
automation_registry_logic_a_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_3/AutomationRegistryLogicA2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_3/AutomationRegistryLogicA2_3.bin 0ec86026840302ec1db31455d039d4e2f93c2646ee48879d644fe489bf873695
automation_registry_logic_a_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_3/AutomationRegistryLogicA2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_3/AutomationRegistryLogicA2_3.bin d2502d6e5d9521f5af71677f9c379598163aa18e822282c8281811b83f7d6188
automation_registry_logic_b_wrapper_2_2: ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_2/AutomationRegistryLogicB2_2.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_2/AutomationRegistryLogicB2_2.bin a6d33dfbbfb0ff253eb59a51f4f6d6d4c22ea5ec95aae52d25d49a312b37a22f
automation_registry_logic_b_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.bin 9f634b949412b06383fe53fd0531a6574e90163855238dbdd3d63589df94b77e
automation_registry_logic_b_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.bin 6c78204720f93e79fff6af600d41ec13076a9aeb22be421179d008cde6b64e9b
automation_registry_wrapper_2_2: ../../contracts/solc/v0.8.19/AutomationRegistry2_2/AutomationRegistry2_2.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_2/AutomationRegistry2_2.bin de60f69878e9b32a291a001c91fc8636544c2cfbd9b507c8c1a4873b602bfb62
automation_registry_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.bin 335dfe64a5cabcf12c6bac6e231919dddec5544a66cd647c229e8fe1f773ab61
automation_registry_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.bin dee0957eb70146a35bfe8d51a21e56daf74c5406a3ce38bb5a57155241fdde86
automation_utils_2_1: ../../contracts/solc/v0.8.16/AutomationUtils2_1/AutomationUtils2_1.abi ../../contracts/solc/v0.8.16/AutomationUtils2_1/AutomationUtils2_1.bin 815b17b63f15d26a0274b962eefad98cdee4ec897ead58688bbb8e2470e585f5
automation_utils_2_2: ../../contracts/solc/v0.8.19/AutomationUtils2_2/AutomationUtils2_2.abi ../../contracts/solc/v0.8.19/AutomationUtils2_2/AutomationUtils2_2.bin 8743f6231aaefa3f2a0b2d484258070d506e2d0860690e66890dccc3949edb2e
automation_utils_2_3: ../../contracts/solc/v0.8.19/AutomationUtils2_3/AutomationUtils2_3.abi ../../contracts/solc/v0.8.19/AutomationUtils2_3/AutomationUtils2_3.bin 11e2b481dc9a4d936e3443345d45d2cc571164459d214917b42a8054b295393b
Expand All @@ -34,7 +34,7 @@ flux_aggregator_wrapper: ../../contracts/solc/v0.6/FluxAggregator/FluxAggregator
gas_wrapper: ../../contracts/solc/v0.8.6/KeeperRegistryCheckUpkeepGasUsageWrapper1_2/KeeperRegistryCheckUpkeepGasUsageWrapper1_2.abi ../../contracts/solc/v0.8.6/KeeperRegistryCheckUpkeepGasUsageWrapper1_2/KeeperRegistryCheckUpkeepGasUsageWrapper1_2.bin 4a5dcdac486d18fcd58e3488c15c1710ae76b977556a3f3191bd269a4bc75723
gas_wrapper_mock: ../../contracts/solc/v0.8.6/KeeperRegistryCheckUpkeepGasUsageWrapper1_2Mock/KeeperRegistryCheckUpkeepGasUsageWrapper1_2Mock.abi ../../contracts/solc/v0.8.6/KeeperRegistryCheckUpkeepGasUsageWrapper1_2Mock/KeeperRegistryCheckUpkeepGasUsageWrapper1_2Mock.bin a9b08f18da59125c6fc305855710241f3d35161b8b9f3e3f635a7b1d5c6da9c8
i_automation_registry_master_wrapper_2_2: ../../contracts/solc/v0.8.19/IAutomationRegistryMaster/IAutomationRegistryMaster.abi ../../contracts/solc/v0.8.19/IAutomationRegistryMaster/IAutomationRegistryMaster.bin 9ff7087179f89f9b05964ebc3e71332fce11f1b8e85058f7b16b3bc0dd6fb96b
i_automation_registry_master_wrapper_2_3: ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.abi ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.bin 461ef24891bd14e5ba956c2bb565715e309809d6e5cb65c39a3a932da7729915
i_automation_registry_master_wrapper_2_3: ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.abi ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.bin d94c75def748323c9628fb5af737f66486a534ea19848dce37f4762c0f507fe0
i_automation_v21_plus_common: ../../contracts/solc/v0.8.19/IAutomationV21PlusCommon/IAutomationV21PlusCommon.abi ../../contracts/solc/v0.8.19/IAutomationV21PlusCommon/IAutomationV21PlusCommon.bin e8a601ec382c0a2e83c49759de13b0622b5e04e6b95901e96a1e9504329e594c
i_chain_module: ../../contracts/solc/v0.8.19/IChainModule/IChainModule.abi ../../contracts/solc/v0.8.19/IChainModule/IChainModule.bin 383611981c86c70522f41b8750719faacc7d7933a22849d5004799ebef3371fa
i_keeper_registry_master_wrapper_2_1: ../../contracts/solc/v0.8.16/IKeeperRegistryMaster/IKeeperRegistryMaster.abi ../../contracts/solc/v0.8.16/IKeeperRegistryMaster/IKeeperRegistryMaster.bin ee0f150b3afbab2df3d24ff3f4c87851efa635da30db04cd1f70cb4e185a1781
Expand Down
Loading