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

Jump to bottom

[M04] Token fees can lead to incorrect calculations #191

Merged
merged 1 commit into from
Jan 11, 2021
Merged

[M04] Token fees can lead to incorrect calculations #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions contracts/Swap.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,11 @@ contract Swap is OwnerPausable, ReentrancyGuard {
}

/**
* @notice calculate amount of tokens you receive on swap
* @notice Calculate amount of tokens you receive on swap
* @param tokenIndexFrom the token the user wants to sell
* @param tokenIndexTo the token the user wants to buy
* @param dx the amount of tokens the user wants to sell
* @param dx the amount of tokens the user wants to sell. If the token charges
* a fee on transfers, use the amount that gets transferred after the fee.
* @return amount of tokens the user will receive
*/
function calculateSwap(uint8 tokenIndexFrom, uint8 tokenIndexTo, uint256 dx
Expand All @@ -233,7 +234,8 @@ contract Swap is OwnerPausable, ReentrancyGuard {
*
* @param amounts an array of token amounts to deposit or withdrawal,
* corresponding to pooledTokens. The amount should be in each
* pooled token's native precision
* pooled token's native precision. If a token charges a fee on transfers,
* use the amount that gets transferred after the fee.
* @param deposit whether this is a deposit or a withdrawal
* @return token amount the user will receive
*/
Expand Down
45 changes: 29 additions & 16 deletions contracts/SwapUtils.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,8 @@ library SwapUtils {
* @notice Externally calculates a swap between two tokens.
* @param tokenIndexFrom the token to sell
* @param tokenIndexTo the token to buy
* @param dx the number of tokens to sell
* @param dx the number of tokens to sell. If the token charges a fee on transfers,
* use the amount that gets transferred after the fee.
* @return dy the number of tokens the user will get
*/
function calculateSwap(
Expand All @@ -486,7 +487,8 @@ library SwapUtils {
*
* @param tokenIndexFrom the token to sell
* @param tokenIndexTo the token to buy
* @param dx the number of tokens to sell
* @param dx the number of tokens to sell. If the token charges a fee on transfers,
* use the amount that gets transferred after the fee.
* @return dy the number of tokens the user will get
* @return dyFee the associated fee
*/
Expand Down Expand Up @@ -558,7 +560,8 @@ library SwapUtils {
*
* @param amounts an array of token amounts to deposit or withdrawal,
* corresponding to pooledTokens. The amount should be in each
* pooled token's native precision
* pooled token's native precision. If a token charges a fee on transfers,
* use the amount that gets transferred after the fee.
* @param deposit whether this is a deposit or a withdrawal
*/
function calculateTokenAmount(
Expand Down Expand Up @@ -615,19 +618,25 @@ library SwapUtils {
uint256 minDy
) external {
require(dx <= self.pooledTokens[tokenIndexFrom].balanceOf(msg.sender), "Cannot swap more than you own");
(uint256 dy, uint256 dyFee) = _calculateSwap(self, tokenIndexFrom, tokenIndexTo, dx);

// Transfer tokens first to see if a fee was charged on transfer
uint256 beforeBalance = self.pooledTokens[tokenIndexFrom].balanceOf(address(this));
self.pooledTokens[tokenIndexFrom].safeTransferFrom(msg.sender, address(this), dx);

// Use the actual transferred amount for AMM math
uint256 transferredDx = self.pooledTokens[tokenIndexFrom].balanceOf(address(this)).sub(beforeBalance);

(uint256 dy, uint256 dyFee) = _calculateSwap(self, tokenIndexFrom, tokenIndexTo, transferredDx);
require(dy >= minDy, "Swap didn't result in min tokens");

uint256 dyAdminFee = dyFee.mul(self.adminFee).div(FEE_DENOMINATOR).div(self.tokenPrecisionMultipliers[tokenIndexTo]);

self.balances[tokenIndexFrom] = self.balances[tokenIndexFrom].add(dx);
self.balances[tokenIndexFrom] = self.balances[tokenIndexFrom].add(transferredDx);
self.balances[tokenIndexTo] = self.balances[tokenIndexTo].sub(dy).sub(dyAdminFee);

self.pooledTokens[tokenIndexFrom].safeTransferFrom(
msg.sender, address(this), dx);
self.pooledTokens[tokenIndexTo].safeTransfer(msg.sender, dy);

emit TokenSwap(msg.sender, dx, dy, tokenIndexFrom, tokenIndexTo);
emit TokenSwap(msg.sender, transferredDx, dy, tokenIndexFrom, tokenIndexTo);
}

/**
Expand All @@ -636,7 +645,7 @@ library SwapUtils {
* @param minToMint the minimum LP tokens adding this amount of liquidity
* should mint, otherwise revert. Handy for front-running mitigation
*/
function addLiquidity(Swap storage self, uint256[] calldata amounts, uint256 minToMint)
function addLiquidity(Swap storage self, uint256[] memory amounts, uint256 minToMint)
external {
require(
amounts.length == self.pooledTokens.length,
Expand All @@ -657,6 +666,17 @@ library SwapUtils {
self.lpToken.totalSupply() != 0 || amounts[i] > 0,
"If token supply is zero, must supply all tokens in pool"
);

// Transfer tokens first to see if a fee was charged on transfer
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 have a test for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I'll add couple test cases for this

if (amounts[i] != 0) {
uint256 beforeBalance = self.pooledTokens[i].balanceOf(address(this));
self.pooledTokens[i].safeTransferFrom(
msg.sender, address(this), amounts[i]);

// Update the amounts[] with actual transfer amount
amounts[i] = self.pooledTokens[i].balanceOf(address(this)).sub(beforeBalance);
}

newBalances[i] = self.balances[i].add(amounts[i]);
}

Expand Down Expand Up @@ -697,13 +717,6 @@ library SwapUtils {
// mint the user's LP tokens
self.lpToken.mint(msg.sender, toMint);

for (uint256 i = 0; i < self.pooledTokens.length; i++) {
if (amounts[i] != 0) {
self.pooledTokens[i].safeTransferFrom(
msg.sender, address(this), amounts[i]);
}
}

emit AddLiquidity(
msg.sender, amounts, fees, D1, self.lpToken.totalSupply()
);
Expand Down