Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ contract BatchConversionPayments is BatchNoConversionPayments {

// Batch sends back to the payer the tokens not spent = excessAmount
// excessAmount = maxToSpend - reallySpent, which is equal to the remaining tokens on the contract
uint256 excessAmount = requestedToken.balanceOf(address(this));
uint256 excessAmount = requestedToken.balanceOf(address(this)) - uTokens[k].tareBalance;
if (excessAmount > 0) {
requestedToken.safeTransfer(msg.sender, excessAmount);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,14 @@ contract BatchNoConversionPayments is Ownable {
address public NativeAddress;
address[][] public pathsNativeToUSD;

/** Contains the address of a token, the sum of the amount and fees paid with it, and the batch fee amount */
struct Token {
address tokenAddress;
// Sum of the amount and fees paid with a token
uint256 amountAndFee;
// Batch fee to pay with this token
uint256 batchFeeAmount;
// Contract balance before the batch call
uint256 tareBalance;
}

/**
Expand Down Expand Up @@ -391,7 +394,7 @@ contract BatchNoConversionPayments is Ownable {
*/
function getUTokens(RequestDetail[] calldata requestDetails)
internal
pure
view
returns (Token[] memory uTokens)
{
// A list of unique tokens, with the sum of maxToSpend by token
Expand All @@ -416,6 +419,9 @@ contract BatchNoConversionPayments is Ownable {
) {
uTokens[k].tokenAddress = rD.path[rD.path.length - 1];

IERC20 paymentToken = IERC20(uTokens[k].tokenAddress);
uTokens[k].tareBalance = paymentToken.balanceOf(address(this));

if (rD.path.length > 1) {
// amountAndFee is used to store _maxToSpend, useful to send enough tokens to this contract
uTokens[k].amountAndFee = rD.maxToSpend;
Expand Down Expand Up @@ -536,6 +542,23 @@ contract BatchNoConversionPayments is Ownable {
return result;
}

/*
* Admin functions for balance handling
*/

/**
* @notice

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the @notice is empty, worth filling in before merging — something like Transfers the entire ERC20 balance of this contract to the specified recipient. Only callable by the owner.

* @param erc20TokenAddress Address of an ERC20 to drain
* @param recipientAddress Address of the receiver
*/
function rescueERC20Funds(address erc20TokenAddress, address recipientAddress)
external
onlyOwner
{
IERC20 token = IERC20(erc20TokenAddress);
token.transfer(recipientAddress, token.balanceOf(address(this)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should use safeTransfer here instead of raw transfer — some ERC20s don't return a boolean on transfer and it'll silently fail. the rest of the contract already uses safeTransfer consistently.

}

/*
* Admin functions to edit the proxies address and fees
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
ChainlinkConversionPath,
TestERC20,
TestERC20__factory,
AggregatorMock__factory,
BatchConversionPayments__factory,
BatchConversionPayments,
} from '../../src/types';
Expand Down Expand Up @@ -45,6 +46,7 @@ describe('contract: BatchConversionPayments', async () => {
const BATCH_FEE = 100; // 1%
const BATCH_DENOMINATOR = 10000;
const daiDecimals = '000000000000000000'; // 18 decimals
const oneDai = '1' + daiDecimals;
const fiatDecimals = '00000000'; // 8 decimals
const thousandWith18Decimal = '1000000000000000000000'; // 21 decimals
const referenceExample = '0xaaaa';
Expand Down Expand Up @@ -448,6 +450,110 @@ describe('contract: BatchConversionPayments', async () => {
expectedFeeFAUBalanceDiff,
);
});
it(`make 1 ERC20 conversion payment, with a pre-existing batch contract balance (user mistake) that must be untouched`, async function () {
// Simulate a user mistake: tokens were accidentally sent directly to the batch
// contract before the payment. This balance must be left untouched.
// We deploy a dedicated ERC20 token for the stranded balance so that its
// leftover on the batch contract does not pollute the other tests
// (FAU balance = 0).
// We refer to this token as "TOK" in this test, although the deployed contract
// has its actual code (ERC20) derived from TestERC20__factory.
const strandedBalance = BigNumber.from(1_234).mul(oneDai);
// The spender must hold (and approve) the converted amount plus fees, with
// margin. 200_000 TOK comfortably covers a $100_000 payment at the FAU rate.
const fromFunding = BigNumber.from(200_000).mul(oneDai);

// Deploy TOK with enough supply for both the spender funding and the stranded
// transfer below. TestERC20 mints the whole supply to the deployer (admin)
// then immediately gives away 10 wei in its constructor, so mint a little
// extra slack to keep admin's balance above what it transfers out.
const tokContract = await new TestERC20__factory(adminSigner).deploy(
fromFunding.add(strandedBalance).add(thousandWith18Decimal),
);
await tokContract.transfer(from, fromFunding);
await tokContract.connect(fromSigner).approve(batchConversionProxy.address, fromFunding.mul(10_000));

// Give TOK a USD rate so the conversion proxy can price it. We reuse the FAU/USD
// rate value: nothing checks that this aggregator actually prices TOK, and the
// expected balances below are computed with the same 'USD_FAU' rate.
const tokAggregator = await new AggregatorMock__factory(adminSigner).deploy(
FAU_USD_RATE,
8,
60,
);
await chainlinkPath
.connect(adminSigner)
.updateAggregator(tokContract.address, USD_hash, tokAggregator.address);

// Strand some TOK directly on the batch contract (the user mistake).
await tokContract.transfer(batchConversionProxy.address, strandedBalance);
expect(await tokContract.balanceOf(batchConversionProxy.address)).to.equals(
strandedBalance,
'pre-existing stranded batch balance',
);

// amountAndFee for a conversion request is taken from maxToSpend (the contract
// pulls in maxToSpend upfront and requires the payer to hold it), so it must be
// sized to TOK, not the giant FAU value. Bigger than the ~50_000 TOK real spend
// so there is genuine excess to refund - which is where the stranded-balance
// bug bites.
const maxToSpend = BigNumber.from(100_000).mul(oneDai).toString();
const tokConvRequest: PaymentTypes.RequestDetail = {
...fauConvRequest,
path: [USD_hash, tokContract.address],
recipient: to,
maxToSpend,
};

const [initialFromTOKBalance, initialToTOKBalance, initialFeeTOKBalance] =
await getERC20Balances(tokContract);
const pathsToUSD = applyLimit
? [
[tokContract.address, USD_hash],
]
: [];
await batchConversionProxy.connect(fromSigner).batchPayments(
[
{
paymentNetworkId: BATCH_PAYMENT_NETWORK_ID.BATCH_MULTI_ERC20_CONVERSION_PAYMENTS,
requestDetails: [tokConvRequest],
},
],
pathsToUSD,
feeAddress,
);
const [fromBalance, toBalance, feeBalance, batchBalance] =
await getERC20Balances(tokContract);

const [, expectedToTOKBalanceDiff] =
getExpectedConvERC20Balances(100000, 100, 1, 'USD_FAU');

// Main test
// The stranded balance is part of the batch contract balance both before and
// after: the payment must flow through without consuming it.
expect(batchBalance.toString()).to.equals(
strandedBalance.toString(),
'stranded batch balance after payment must be untouched',
);

// Sanity checks
expect(toBalance.sub(initialToTOKBalance)).to.equals(
expectedToTOKBalanceDiff,
'(sanity) Receiver should receive the request amount',
);
expect(feeBalance.sub(initialFeeTOKBalance).gt(0),
'(sanity) Fee address should receive fees',
);
Comment on lines +544 to +546

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this expect() is missing a .to.be.true at the end — without it the call is a no-op and this sanity check never actually asserts anything

expect(fromBalance.sub(initialFromTOKBalance).gt(0),
'(sanity) Spender should spend',
);
Comment on lines +547 to +549

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here — missing .to.be.true. also fromBalance.sub(initialFromTOKBalance) will throw a BigNumber underflow since the spender balance decreases after payment, probably needs initialFromTOKBalance.sub(fromBalance).gt(0).to.be.true

// The real spend must be strictly below maxToSpend, otherwise there is no
// excess slack and the stranded balance could not be touched at all.
expect(
fromBalance.sub(initialFromTOKBalance).lt(BigNumber.from(maxToSpend)),
'(sanity) Real spend must be below maxToSpend to leave excess slack',
).to.be.true;
});
it('make 3 ERC20 payments with different tokens and conversion lengths', async () => {
const batchPayments = async () => {
return await batchConversionProxy.batchPayments(
Expand Down Expand Up @@ -1002,5 +1108,34 @@ describe('contract: BatchConversionPayments', async () => {
initialFeeETHBalance,
);
});
describe('rescueERC20Funds', () => {
it('lets the owner drain a stranded ERC20 balance to a recipient', async () => {
// A dedicated token, so a leftover does not pollute other tests' balances.
const stranded = BigNumber.from(1_234).mul(oneDai);
const token = await new TestERC20__factory(adminSigner).deploy(
stranded.add(thousandWith18Decimal),
);
await token.transfer(batchConversionProxy.address, stranded);
expect(await token.balanceOf(batchConversionProxy.address)).to.equals(stranded);

const initialRecipientBalance = await token.balanceOf(to);
await batchConversionProxy.connect(adminSigner).rescueERC20Funds(token.address, to);

expect(await token.balanceOf(batchConversionProxy.address)).to.equals(
0,
'contract balance must be fully drained',
);
expect(await token.balanceOf(to)).to.equals(
initialRecipientBalance.add(stranded),
'recipient must receive the whole drained balance',
);
});
it('reverts when called by a non-owner', async () => {
await expect(
// batchConversionProxy is connected to fromSigner, a non-owner.
batchConversionProxy.rescueERC20Funds(fauERC20.address, to),
).to.be.revertedWith('Ownable: caller is not the owner');
});
});
});
});
Loading