5-t-swap-audit's People
Forkers
0xjuaan niket-yende libaice taha-zerarkaa surfer05 alkautsarf acidrums7 marindimitrov12 masami87 canberkege allwin199 minigas issa99-lab weber77 hebx alaza747 swapnapratim fabriziogianni7 dmg-01 dsdo01 jbuildsdev kaustubh76 cheshiresec profsaz5-t-swap-audit's Issues
testing errors
when i use :pragma solidity 0.8.20; i get
Source file requires different compiler version (current compiler is 0.8.19+commit.7dd6d404.Emscripten.clang) - note that nightly builds are considered to be strictly less than the released version
when i change to: pragma solidity >=0.8.19 <0.9.0;
i get an error with my: import {Test} from "forge-std/Test.sol";
Source "forge-std/Test.sol" not found: File import callback not supported
please help
PoC on `sellPoolTokens` to demonstrate its validity as high-risk audit finding
While I'm reviewing the updraft content Smart Contract Security and Auditing
on section 5 TSwap
part 44 Reporting: sellPoolToken
,
/**
* @notice wrapper function to facilitate users selling pool tokens in exchange of WETH
* @param poolTokenAmount amount of pool tokens to sell
* @return wethAmount amount of WETH received by caller
*/
function sellPoolTokens(
uint256 poolTokenAmount
) external returns (uint256 wethAmount) {
// @audit: this is wrong [ according to updraft video walk-through ]
// should return swapExactInput(minWethToReceive) instead
return
swapExactOutput(
i_poolToken,
i_wethToken,
poolTokenAmount,
uint64(block.timestamp)
);
}
there wasn't a demo walk-through for the PoC. Therefore, I'm attempting to write the PoC to complete the reporting for the audit finding on sellPoolToken
. However in this attempt, my PoC shows that the original returned function swapExactOutput
in sellPoolToken
is correct
<@@>!?, which is contradicting that this is a high risk audit violation as the rightful returned function is supposed to be swapExactInput
according to course content.
I have put down my thought process and validation workflow of the PoC below. Kindly assist to review :
In TSwapPool.t.sol
, my test case was written as follow
// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
import { Test, console } from "forge-std/Test.sol";
import { TSwapPool } from "../../src/PoolFactory.sol";
import { ERC20Mock } from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
import { IERC20 } from "@openzeppelin/contracts/interfaces/IERC20.sol";
contract TSwapPoolTest is Test {
TSwapPool pool;
ERC20Mock poolToken;
ERC20Mock weth;
address liquidityProvider = makeAddr("liquidityProvider");
address user = makeAddr("user");
function setUp() public {
poolToken = new ERC20Mock();
weth = new ERC20Mock();
pool = new TSwapPool(address(poolToken), address(weth), "LTokenA", "LA");
weth.mint(liquidityProvider, 200e18);
poolToken.mint(liquidityProvider, 200e18);
weth.mint(user, 10e18);
poolToken.mint(user, 10e18);
}
function testSellPoolToken() public {
// injecting funds into pool by liquidity provider
vm.startPrank(liquidityProvider);
weth.approve(address(pool), 100e18);
poolToken.approve(address(pool), 100e18);
pool.deposit(100e18, 100e18, 100e18, uint64(block.timestamp));
vm.stopPrank();
// starting pranking that a user want to sell his pool token
vm.startPrank(user);
uint256 balanceWethB4Sell = weth.balanceOf(user); // will return 10e18 as minted in setUp
uint256 balancePoolTokenB4Sell = poolToken.balanceOf(user); // will return 10e18 as minted in setUp
console.log("balance weth_user before sell: ", balanceWethB4Sell);
console.log("balance poolToken_user before sell: ", balancePoolTokenB4Sell);
poolToken.approve(address(pool), 10e18);
// assuming user only want to sell 1e16 poolToken out of his original poolToken amount of 10e18
uint256 poolTokenAmountToSell = 1e16;
console.log("poolTokenAmountToSell: ", poolTokenAmountToSell);
// Prerequisite: correct back another audit finding ( change 10000 to 1000) on `getInputAmountBasedOnOutput` under the `swapExactOutput`, so that we only test `swapExactOutput` validity in the `sellPoolToken` function and not influenced by the error caused by other factor.
// as formulated in `getInputAmountBasedOnOutput`, calculatedWethToReceive = ((100e18 * 1e16) * uint256(1000)) / ((100e18 - 1e16) * uint256(997))
uint256 calculatedWethToReceive = pool.sellPoolTokens(poolTokenAmountToSell);
console.log("calculatedWethToReceive: ", calculatedWethToReceive);
// find out how much fee is collected by protocol from the transaction
// the difference of calculatedWethToReceive and poolTokenAmountToSell will be the fee collected by protocol
// since formula ((100e18 * 1e16) * uint256(1000)) / ((100e18 - 1e16) * uint256(997)) returns weth that includes fee
uint256 feeChargedByProtocol = calculatedWethToReceive - poolTokenAmountToSell;
console.log("feeChargedByProtocol: ", feeChargedByProtocol);
uint256 balanceWethAfterSell = weth.balanceOf(user);
uint256 balancePoolTokenAfterSell = poolToken.balanceOf(user);
// tapout the expected balance of weth and poolToken after the sell
// factoring in the fee charged by protocol for the execution of the transaction
uint256 expectedBalanceWethAftSell = balanceWethB4Sell + calculatedWethToReceive - feeChargedByProtocol;
uint256 expectedBalancePoolTokenAftSell = balancePoolTokenB4Sell - poolTokenAmountToSell - feeChargedByProtocol;
console.log("balance weth_user after sell: ", balanceWethAfterSell);
console.log("expectedBalanceWethAftSell: ", expectedBalanceWethAftSell);
console.log("balance poolToken_user after sell: ", balancePoolTokenAfterSell);
console.log("expectedPoolTokenBalanceAftSell: ", expectedBalancePoolTokenAftSell);
console.log("poolTokenBalanceIfNoFeeImposed: ", balancePoolTokenB4Sell - poolTokenAmountToSell);
assertEq(balanceWethAfterSell, expectedBalanceWethAftSell);
assertEq(balancePoolTokenAfterSell, expectedBalancePoolTokenAftSell);
}
}
After this test, if returned function swapExactOutput
in sellPoolToken
is not correct, then this test should fail, indicating that swapExactOutput
is not the rightful returned function for sellPoolToken
, which then considered as a high risk vulnerability.
However, this test indeed passed. The logs printed via console shown as below:
balance weth_user before sell: 10000000000000000000
balance poolToken_user before sell: 10000000000000000000
poolTokenAmountToSell: 10000000000000000
calculatedWethToReceive: 10031093380150452
feeChargedByProtocol: 31093380150452
balance weth_user after sell: 10010000000000000000
expectedBalanceWethAftSell: 10010000000000000000
balance poolToken_user after sell: 9989968906619849548
expectedPoolTokenBalanceAftSell: 9989968906619849548
poolTokenBalanceIfNoFeeImposed: 9990000000000000000
From the values in the logs, we can see that the final weth in user account ( balance weth_user after sell
) did increase to the exact output the user want as the function does/named swapExactOutput
( increased by exactly 1e16 in the test case ) and the corresponding poolToken did decrease from its initial balance (from 10e18 to 9989968906619849548 ) after deduction of the fee charged by the protocol.
The whole transaction by the protocol only incurred 1 time fee charge, which deducted from the poolToken and keep the final returned weth to the output increase by exactly 1e16 as specified upon executing the sellPoolToken
via its parameter poolTokenAmount
. Therefore, the returned function swapExactOutput
under sellPoolToken
seems correct, and not an audit risk.. and the TSwap protocol does charge one time fee only ( as compared to conventional order book system ), which tally to state why TSwap is favorable as it's decentralized and cost saving.
I could have misunderstood / misinterprete how this PoC should be written. As there is no demo-PoC to show that this is a high risk vulnerability, I couldn't find the right reasoning how the course content relates this sellPoolToken
as a high risk audit finding if using my own PoC above. May I check with the expert here for further comments or explanation?
Please don't hesitate to highlight any mistakes above as I'm still learning and exploring the topic. Feedbacks are greatly welcomed. Thanks.
Doubt in the math to calculate the `poolTokensToDeposit` in the `deposit()` function
According to the comments, the math explanation considers this invariant to hold:
// Our invariant says weth, poolTokens, and liquidity tokens must always have the same ratio after the
// initial deposit
// poolTokens / constant(k) = weth
// weth / constant(k) = liquidityTokens
// aka...
// weth / poolTokens = constant(k)
Refer code: https://github.com/Cyfrin/5-t-swap-audit/blob/main/src/TSwapPool.sol#L132
But for a constant product AMM, the invariant formula is
poolTokens * wEth = constant(k)
Then why is poolTokens / constant(k) = weth
being used?
This being said, the formula for calculating poolTokenToDeposit
is correct in the getPoolTokensToDepositBasedOnWeth()
https://github.com/Cyfrin/5-t-swap-audit/blob/main/src/TSwapPool.sol#L429
The comment I pointed out seems a bit confusing/contradicting to what is actually being used in getPoolTokensToDepositBasedOnWeth()
.
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ๐๐๐
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google โค๏ธ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.