Platform: Code4rena
Start Date: 04/01/2022
Pot Size: $75,000 USDC
Total HM: 17
Participants: 33
Period: 7 days
Judge: 0xean
Total Solo HM: 14
Id: 74
League: ETH
Rank: 2/33
Findings: 6
Award: $15,672.59
🌟 Selected for report: 18
🚀 Solo Findings: 3
🌟 Selected for report: WatchPug
5679.4846 USDC - $5,679.48
WatchPug
In the current implementation, borrow() takes a user input value of zIncrease, while the actual collateral asset transferred in is calculated at L319, the state of pool.state.z still increased by the value of the user's input at L332.
Even though a large number of zIncrease means that the user needs to add more collateral, the attacker can use a dust amount xDecrease (1 wei for example) so that the total collateral needed is rather small.
Plus, the attacker can always pay() the dust amount of loan to get back the rather large amount of collateral added.
function borrow( uint256 maturity, address assetTo, address dueTo, uint112 xDecrease, uint112 yIncrease, uint112 zIncrease, bytes calldata data ) external override lock returns (uint256 id, Due memory dueOut) { require(block.timestamp < maturity, 'E202'); require(assetTo != address(0) && dueTo != address(0), 'E201'); require(assetTo != address(this) && dueTo != address(this), 'E204'); require(xDecrease > 0, 'E205'); Pool storage pool = pools[maturity]; require(pool.state.totalLiquidity > 0, 'E206'); BorrowMath.check(pool.state, xDecrease, yIncrease, zIncrease, fee); dueOut.debt = BorrowMath.getDebt(maturity, xDecrease, yIncrease); dueOut.collateral = BorrowMath.getCollateral(maturity, pool.state, xDecrease, zIncrease); dueOut.startBlock = BlockNumber.get(); Callback.borrow(collateral, dueOut.collateral, data); id = pool.dues[dueTo].insert(dueOut); pool.state.reserves.asset -= xDecrease; pool.state.reserves.collateral += dueOut.collateral; pool.state.totalDebtCreated += dueOut.debt; pool.state.x -= xDecrease; pool.state.y += yIncrease; pool.state.z += zIncrease; asset.safeTransfer(assetTo, xDecrease); emit Sync(maturity, pool.state.x, pool.state.y, pool.state.z); emit Borrow(maturity, msg.sender, assetTo, dueTo, xDecrease, id, dueOut); }
function getCollateral( uint256 maturity, IPair.State memory state, uint112 xDecrease, uint112 zIncrease ) internal view returns (uint112 collateralIn) { uint256 _collateralIn = maturity; _collateralIn -= block.timestamp; _collateralIn *= zIncrease; _collateralIn = _collateralIn.shiftRightUp(25); uint256 minimum = state.z; minimum *= xDecrease; uint256 denominator = state.x; denominator -= xDecrease; minimum = minimum.divUp(denominator); _collateralIn += minimum; collateralIn = _collateralIn.toUint112(); }
Near the maturity time, the attacker can do the following:
borrow() a dust amount of assets (xDecrease = 1 wei) and increase pool.state.z to an extremely large value (20x of previous state.z in our tests);pay() the loan and get back the collateral;lend() a regular amount of state.x, get a large amount of insurance token;burn() the insurance token and get a large portion of the collateral assets from the defaulted loans.Consider making pair.borrow() to be onlyConvenience, so that zIncrease will be a computed value (based on xDecrease and current state) rather than a user input value.
🌟 Selected for report: WatchPug
5679.4846 USDC - $5,679.48
WatchPug
This issue is similar to the two previous issues related to state.y manipulation. Unlike the other two issues, this function is not on TimeswapPair.sol but on TimeswapConvenience.sol, therefore this can not be solved by adding onlyConvenience modifier.
Actually, we believe that it does not make sense for the caller to specify the interest they want to pay, we recommend removing this function.
pool.state.y is extremely large, many core features of the protocol will malfunction, as the arithmetic related to state.y can overflow. For example:LendMath.check(): https://github.com/code-423n4/2022-01-timeswap/blob/bf50d2a8bb93a5571f35f96bd74af54d9c92a210/Timeswap/Timeswap-V1-Core/contracts/libraries/LendMath.sol#L28-L28
BorrowMath.check(): https://github.com/code-423n4/2022-01-timeswap/blob/bf50d2a8bb93a5571f35f96bd74af54d9c92a210/Timeswap/Timeswap-V1-Core/contracts/libraries/BorrowMath.sol#L31-L31
state.y to a near overflow value, then lend() to get a large amount of extra interest (as Bond tokens) with a small amount of asset tokens. This way, the attacker can steal funds from other lenders and liquidity providers.🌟 Selected for report: Rhynorater
Also found by: WatchPug, harleythedog, hyh
WatchPug
pool.state.y += yIncrease;
TimeswapPair.sol#mint() takes a user input value of yIncrease without proper validation, which means that it allows the state of pool.state.y to increase by the arbitrary value set by the caller.
pool.state.y is extremely large, many core features of the protocol will malfunction, as the arithmetic related to state.y can overflow. For example:LendMath.check(): https://github.com/code-423n4/2022-01-timeswap/blob/bf50d2a8bb93a5571f35f96bd74af54d9c92a210/Timeswap/Timeswap-V1-Core/contracts/libraries/LendMath.sol#L28-L28
BorrowMath.check(): https://github.com/code-423n4/2022-01-timeswap/blob/bf50d2a8bb93a5571f35f96bd74af54d9c92a210/Timeswap/Timeswap-V1-Core/contracts/libraries/BorrowMath.sol#L31-L31
state.y to a near overflow value, then lend() to get a large amount of extra interest (as Bond tokens) with a small amount of asset tokens. This way, the attacker can steal funds from other lenders and liquidity providers.Near the maturity time, the attacker can do the following:
mint() with a dust amount of assets (xIncrease = 1 wei) and increase pool.state.y to an extremely large value;lend() a regular amount of assets, get a large amount of bond token;burn() the bond token and get a large portion of the assets.Consider making pair.mint() to be onlyConvenience, so that yIncrease will be a computed value (based on xIncrease and current state) rather than a user input value.
#0 - Mathepreneur
2022-01-24T12:36:11Z
Duplicate of #187
🌟 Selected for report: WatchPug
1703.8454 USDC - $1,703.85
WatchPug
The current implementation of TimeswapPair.sol#mint() allows the caller to specify an arbitrary value for yIncrease.
However, since state.y is expected to be a large number based at 2**32, once the initial state.y is set to a small number (1 wei for example), the algorithm won't effectively change state.y with regular market operations (borrow, lend and mint).
The pair with the maturity will malfunction and can only be abandoned.
A malicious user/attacker can use this to frontrun other users or the platform's newLiquidity() call to initiate a griefing attack.
If the desired maturity is a meaningful value for the user/platform, eg, end of year/quarter. This can be a noteworthy issue.
Consider adding validation of minimal state.y for new liquidity.
Can be 2**32 / 10000 for example.
🌟 Selected for report: WatchPug
567.9485 USDC - $567.95
WatchPug
function sqrtUp(uint256 y) internal pure returns (uint256 z) { z = sqrt(y); if (z % y > 0) z++; }
For example, when y = 9:
z % y > 0 is true, therefore, z++, z is 4Expected Results: sqrtUp(9) = 4
Actual Results: sqrtUp(9) = 3
Change to:
function sqrtUp(uint256 y) internal pure returns (uint256 z) { z = sqrt(y); if (z * z < y) ++z; }
or
function sqrtUp(uint256 y) internal pure returns (uint256 z) { z = sqrt(y); if (y % z != 0) ++z; }
#0 - Mathepreneur
2022-01-17T13:19:09Z
9.1606 USDC - $9.16
WatchPug
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Instances include:
TimeswapPair.sol#pay()
PayMath.sol#givenMaxAssetsIn()
#0 - Mathepreneur
2022-01-18T10:42:54Z
🌟 Selected for report: WatchPug
93.0809 USDC - $93.08
WatchPug
if (pool.state.totalLiquidity == 0) { uint256 liquidityTotal = MintMath.getLiquidityTotal(xIncrease); liquidityOut = MintMath.getLiquidity(maturity, liquidityTotal, protocolFee); pool.state.totalLiquidity += liquidityTotal; pool.liquidities[factory.owner()] += liquidityTotal - liquidityOut; }
At L157, we know that pool.state.totalLiquidity == 0, therefore at L161 += can be replaced with =.
Using = directly can avoid unnecessary storage read of pool.state.totalLiquidity and save some gas.
Change to:
if (pool.state.totalLiquidity == 0) { uint256 liquidityTotal = MintMath.getLiquidityTotal(xIncrease); liquidityOut = MintMath.getLiquidity(maturity, liquidityTotal, protocolFee); pool.state.totalLiquidity = liquidityTotal; pool.liquidities[factory.owner()] += liquidityTotal - liquidityOut; }
#0 - Mathepreneur
2022-01-18T10:39:16Z
The refactor makes this unnecessary.
🌟 Selected for report: WatchPug
93.0809 USDC - $93.08
WatchPug
if (pool.state.totalLiquidity == 0) { uint256 liquidityTotal = MintMath.getLiquidityTotal(xIncrease); liquidityOut = MintMath.getLiquidity(maturity, liquidityTotal, protocolFee); pool.state.totalLiquidity += liquidityTotal; pool.liquidities[factory.owner()] += liquidityTotal - liquidityOut; } else { uint256 liquidityTotal = MintMath.getLiquidityTotal(pool.state, xIncrease, yIncrease, zIncrease); liquidityOut = MintMath.getLiquidity(maturity, liquidityTotal, protocolFee); pool.state.totalLiquidity += liquidityTotal; pool.liquidities[factory.owner()] += liquidityTotal - liquidityOut; }
Change to:
uint256 liquidityTotal = pool.state.totalLiquidity == 0 ? MintMath.getLiquidityTotal(xIncrease) : MintMath.getLiquidityTotal(pool.state, xIncrease, yIncrease, zIncrease); liquidityOut = MintMath.getLiquidity(maturity, liquidityTotal, protocolFee); pool.state.totalLiquidity += liquidityTotal; pool.liquidities[factory.owner()] += liquidityTotal - liquidityOut;
#0 - Mathepreneur
2022-01-17T16:09:48Z
🌟 Selected for report: WatchPug
Also found by: Dravee, PPrieditis, csanuragjain, gzeon
12.2141 USDC - $12.21
WatchPug
Move storage writes to inside the code block of if (tokensOut.asset > 0) {...} can avoid unnecessary code execution when this check doesn't pass and save gas.
pool.state.reserves.asset -= tokensOut.asset; pool.state.reserves.collateral -= tokensOut.collateral; if (tokensOut.asset > 0) asset.safeTransfer(assetTo, tokensOut.asset); if (tokensOut.collateral > 0) collateral.safeTransfer(collateralTo, tokensOut.collateral);
Change to:
if (tokensOut.asset > 0) { pool.state.reserves.asset -= tokensOut.asset; asset.safeTransfer(assetTo, tokensOut.asset); } if (tokensOut.collateral > 0) { pool.state.reserves.collateral -= tokensOut.collateral; collateral.safeTransfer(collateralTo, tokensOut.collateral); }
#0 - Mathepreneur
2022-01-17T14:09:48Z
25.1318 USDC - $25.13
WatchPug
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
For example:
require(block.timestamp < maturity, 'E202'); require(maturity - block.timestamp < 0x100000000, 'E208');
maturity - block.timestamp will never underflow.
if (state.reserves.asset >= state.totalClaims.bond) return collateralOut; uint256 deficit = state.totalClaims.bond; deficit -= state.reserves.asset;
deficit -= state.reserves.asset will never underflow.
if (msg.value > value) ETH.transfer(payable(msg.sender), msg.value - value);
msg.value - value will never underflow.
#0 - Mathepreneur
2022-01-17T14:06:34Z
🌟 Selected for report: WatchPug
93.0809 USDC - $93.08
WatchPug
flag == 0 is cheaper than temp % 10 == 0.
Therefore, checking flag == 0 first can save some gas.
if (temp % 10 == 0 && flag == 0)
Change to:
if (flag == 0 && temp % 10 == 0)
Other instances include:
else if (value % 10 != 0 && flag == 0)
#0 - Mathepreneur
2022-01-18T10:25:21Z
🌟 Selected for report: WatchPug
93.0809 USDC - $93.08
WatchPug
import {IPair} from '../interfaces/IPair.sol';
IPair is unused in Callback.sol.
#0 - Mathepreneur
2022-01-18T10:34:07Z
🌟 Selected for report: WatchPug
93.0809 USDC - $93.08
WatchPug
function isValidDate(uint year, uint month, uint day) internal pure returns (bool valid) { if (year >= 1970 && month > 0 && month <= 12) { uint daysInMonth = _getDaysInMonth(year, month); if (day > 0 && day <= daysInMonth) { valid = true; } } }
The local variable daysInMonth is used only once. Making the expression inline can save gas.
Change to:
function isValidDate(uint year, uint month, uint day) internal pure returns (bool valid) { if (year >= 1970 && month > 0 && month <= 12) { if (day > 0 && day <= _getDaysInMonth(year, month)) { valid = true; } } }
Other examples include:
IPair pair = factory.getPair(params.asset, params.collateral); require(address(pair) != address(0), 'E501');
IDue collateralizedDebt = natives[asset][collateral][maturity].collateralizedDebt; require(msg.sender == address(collateralizedDebt), 'E701');
uint256 _assetReserve = asset.safeBalance(); require(_assetReserve >= assetReserve + assetIn, 'E304');
uint256 _collateralReserve = collateral.safeBalance(); require(_collateralReserve >= collateralReserve + collateralIn, 'E305');
#0 - Mathepreneur
2022-01-18T10:21:28Z
🌟 Selected for report: WatchPug
93.0809 USDC - $93.08
WatchPug
checkProportional() is a rather simple one line function, making it inline instead of an internal function call can make the code simpler and save some gas.
for (uint256 i; i < ids.length; i++) { Due storage due = dues[ids[i]]; require(due.startBlock != BlockNumber.get(), 'E207'); if (owner != msg.sender) require(collateralsOut[i] == 0, 'E213'); PayMath.checkProportional(assetsIn[i], collateralsOut[i], due); due.debt -= assetsIn[i]; due.collateral -= collateralsOut[i]; assetIn += assetsIn[i]; collateralOut += collateralsOut[i]; }
function checkProportional( uint112 assetIn, uint112 collateralOut, IPair.Due memory due ) internal pure { require(uint256(assetIn) * due.collateral >= uint256(collateralOut) * due.debt, 'E303'); } }
Can be changed to:
for (uint256 i; i < ids.length; i++) { Due storage due = dues[ids[i]]; require(due.startBlock != BlockNumber.get(), 'E207'); if (owner != msg.sender) require(collateralsOut[i] == 0, 'E213'); require(uint256(assetIn[i]) * due.collateral >= uint256(collateralOut[i]) * due.debt, 'E303'); due.debt -= assetsIn[i]; due.collateral -= collateralsOut[i]; assetIn += assetsIn[i]; collateralOut += collateralsOut[i]; }
#0 - Mathepreneur
2022-01-17T13:51:21Z
9.1606 USDC - $9.16
WatchPug
There is no risk of overflow caused by increamenting the iteration index in for loops (the i++ in for for (uint256 i; i < ids.length; i++)).
Increments perform overflow checks that are not necessary in this case.
Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the for loop:
for (uint256 i; i < ids.length; i++) { IPair.Due memory due = pair.dueOf(maturity, address(collateralizedDebt), ids[i]); if (assetsIn[i] > due.debt) assetsIn[i] = due.debt; if (msg.sender == collateralizedDebt.ownerOf(ids[i])) { uint256 _collateralOut = due.collateral; if (due.debt > 0) { _collateralOut *= assetsIn[i]; _collateralOut /= due.debt; } collateralsOut[i] = _collateralOut.toUint112(); } }
to:
for (uint256 i; i < ids.length;) { IPair.Due memory due = pair.dueOf(maturity, address(collateralizedDebt), ids[i]); if (assetsIn[i] > due.debt) assetsIn[i] = due.debt; if (msg.sender == collateralizedDebt.ownerOf(ids[i])) { uint256 _collateralOut = due.collateral; if (due.debt > 0) { _collateralOut *= assetsIn[i]; _collateralOut /= due.debt; } collateralsOut[i] = _collateralOut.toUint112(); } unchecked { ++i; } }
#0 - Mathepreneur
2022-01-17T20:24:22Z
41.8864 USDC - $41.89
WatchPug
Every reason string takes at least 32 bytes.
Use short reason strings that fits in 32 bytes or it will become more expensive.
Instances include:
require( owner == msg.sender || isApprovedForAll[owner][msg.sender], 'ERC721 :: approve : Approve caller is not owner nor approved for all' );
require( _checkOnERC721Received(address(0), to, id, ''), 'ERC721 :: _safeMint : Transfer to non ERC721Receiver implementer' );
#0 - Mathepreneur
2022-01-17T13:46:16Z
WatchPug
It's cheaper to use != 0 than > 0 for uint256.
require(liquidityOut > 0, 'E212');
require(xIncrease > 0 && yIncrease > 0 && zIncrease > 0, 'E205');
require(liquidityIn > 0, 'E205');
if (tokensOut.asset > 0) asset.safeTransfer(assetTo, tokensOut.asset); if (tokensOut.collateral > 0) collateral.safeTransfer(collateralTo, tokensOut.collateral);
require(xIncrease > 0, 'E205');
require(pool.state.totalLiquidity > 0, 'E206');
require(claimsIn.bond > 0 || claimsIn.insurance > 0, 'E205');
#0 - amateur-dev
2022-01-14T11:05:39Z
Similar issue reported over #172, hence closing this
🌟 Selected for report: WatchPug
93.0809 USDC - $93.08
WatchPug
Check input value earlier can avoid unnecessary code execution when this check failed.
function toUint128(uint256 x) internal pure returns (uint128 y) { require((y = uint128(x)) == x); }
Change to:
function toUint128(uint256 value) internal pure returns (uint128) { require(value <= type(uint128).max, "SafeCast: value doesn't fit in 128 bits"); return uint128(value); }
SafeCast.sol#toUint112() got the similar issue.
#0 - Mathepreneur
2022-01-17T13:24:15Z
🌟 Selected for report: WatchPug
93.0809 USDC - $93.08
WatchPug
The check of y > 3 is unnecessary and most certainly adds more gas cost than it saves as the majority of use cases of this function will not be handling y <= 3.
function sqrt(uint256 y) internal pure returns (uint256 z) { if (y > 3) { z = y; uint256 x = y / 2 + 1; while (x < z) { z = x; x = (y / x + x) / 2; } } else if (y != 0) { z = 1; } }
Change to:
function sqrt(uint x) public pure returns (uint y) { uint z = (x + 1) / 2; y = x; while (z < y) { y = z; z = (x / z + z) / 2; } }
Or use:
#0 - Mathepreneur
2022-01-18T10:14:01Z
🌟 Selected for report: WatchPug
93.0809 USDC - $93.08
WatchPug
if (significantDigits > 10 ** 9) {
Can be changed to:
if (significantDigits > 1e9) {
#0 - Mathepreneur
2022-01-17T13:15:11Z
🌟 Selected for report: Rhynorater
Also found by: WatchPug
41.8864 USDC - $41.89
WatchPug
function getAsset(IPair.State memory state, uint256 liquidityIn) internal pure returns (uint128 assetOut) { if (state.reserves.asset <= state.totalClaims.bond) return assetOut; uint256 _assetOut = state.reserves.asset; _assetOut -= state.totalClaims.bond; _assetOut = _assetOut.mulDiv(liquidityIn, state.totalLiquidity); assetOut = _assetOut.toUint128(); }
Change to:
function getAsset(IPair.State memory state, uint256 liquidityIn) internal pure returns (uint128) { if (state.reserves.asset <= state.totalClaims.bond) return 0; unchecked { return (uint256(state.reserves.asset) - state.totalClaims.bond).mulDiv(liquidityIn, state.totalLiquidity).toUint128(); } }
_assetOut, assetOutunchecked to avoid unnecessary underflow checks#0 - Mathepreneur
2022-01-18T10:08:38Z
Duplicate of #183
25.1318 USDC - $25.13
WatchPug
modifier lock() { require(locked == 0, 'E211'); locked = 1; _; locked = 0; }
SSTORE from 0 to 1 (or any non-zero value), the cost is 20000;
SSTORE from 1 to 2 (or any other non-zero value), the cost is 5000.
By storing the original value once again, a refund is triggered (https://eips.ethereum.org/EIPS/eip-2200).
Since refunds are capped to a percentage of the total transaction's gas, it is best to keep them low, to increase the likelihood of the full refund coming into effect.
Therefore, switching between 1, 2 instead of 0, 1 will be more gas efficient.
#0 - amateur-dev
2022-01-15T03:58:42Z
Similar issue reported over here #87; hence closing this