Timeswap contest - Dravee's results

Handle

Dravee

Vulnerability details

Impact

Increased gas cost

Proof of Concept

In PayMath.sol:givenMaxAssetsIn(), it's possible to save gas by breaking out of the for-loop when the right collateralsOut[i] is updated:

21:         for (uint256 i; i < ids.length; i++) {
22:             IPair.Due memory due = pair.dueOf(maturity, address(collateralizedDebt), ids[i]);
23: 
24:             if (assetsIn[i] > due.debt) assetsIn[i] = due.debt;
25:             if (msg.sender == collateralizedDebt.ownerOf(ids[i])) {
26:                 uint256 _collateralOut = due.collateral;
27:                 if (due.debt > 0) {
28:                     _collateralOut *= assetsIn[i];
29:                     _collateralOut /= due.debt;
30:                 }
31:                 collateralsOut[i] = _collateralOut.toUint112(); //@audit consider adding a break statement after that
32:             }
33:         }

Tools Used

VS Code

Recommended Mitigation Steps

Apply the refacto

Handle

Dravee

Vulnerability details

Impact

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.

Proof of Concept

// Array length:
Timeswap-V1-Convenience\contracts\libraries\NFTTokenURIScaffold.sol:201:        for (uint256 i = 0; i < data.length; i++) {
Timeswap-V1-Convenience\contracts\libraries\PayMath.sol:21:        for (uint256 i; i < ids.length; i++) {
Timeswap-V1-Core\contracts\TimeswapPair.sol:359:        for (uint256 i; i < ids.length; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Store the array's length in a variable before the for-loop, and use it instead.

Handle

Dravee

Vulnerability details

Impact

Duplicated code, loss of maintainability, increased contract size which leads to increased gas cost

Proof of Concept

The following can be simplified in TimeswapPair.sol:mint:

        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;
        }

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;

or even

        uint256 liquidityTotal;
        if (pool.state.totalLiquidity == 0) {
            liquidityTotal = MintMath.getLiquidityTotal(xIncrease);
        } else {
            liquidityTotal = MintMath.getLiquidityTotal(pool.state, xIncrease, yIncrease, zIncrease);
        }
        liquidityOut = MintMath.getLiquidity(maturity, liquidityTotal, protocolFee);

        pool.state.totalLiquidity += liquidityTotal;
        pool.liquidities[factory.owner()] += liquidityTotal - liquidityOut;

Tools Used

VS Code

Recommended Mitigation Steps

Apply the refacto and look out for duplicated code

Handle

Dravee

Vulnerability details

Impact

Increased gas cost.

Proof of Concept

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Let's look at these lines:

Timeswap-V1-Core\contracts\TimeswapPair.sol:162:            pool.liquidities[factory.owner()] += liquidityTotal - liquidityOut;
Timeswap-V1-Core\contracts\TimeswapPair.sol:168:            pool.liquidities[factory.owner()] += liquidityTotal - liquidityOut;

Here, in MintMath.getLiquidity:L54: liquidityOut is set to a value <= liquidityTotal. Therefore liquidityTotal - liquidityOut cannot underflow.

Tools Used

VS Code

Recommended Mitigation Steps

Uncheck arithmetic operations when possible.

Handle

Dravee

Vulnerability details

Impact

++i costs less gass compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Proof of Concept

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

Timeswap-V1-Core\contracts\TimeswapPair.sol:359:        for (uint256 i; i < ids.length; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Use ++i instead of i++ to increment the value of an uint variable.

Handle

Dravee

Vulnerability details

Impact

!= 0 costs less gas compared to > 0 for unsigned integer

Proof of Concept

> 0 is used in the following location(s):

Timeswap-V1-Convenience\contracts\base\ERC721.sol:147:            } else if (_return.length > 0) {
Timeswap-V1-Convenience\contracts\libraries\Burn.sol:40:        if (tokensOut.asset > 0) {
Timeswap-V1-Convenience\contracts\libraries\Burn.sol:65:        if (tokensOut.collateral > 0) {
Timeswap-V1-Convenience\contracts\libraries\DateTime.sol:128:        if (year >= 1970 && month > 0 && month <= 12) {
Timeswap-V1-Convenience\contracts\libraries\DateTime.sol:130:            if (day > 0 && day <= daysInMonth) {
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:296:        require(pair.totalLiquidity(params.maturity) > 0, 'E507');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:455:        require(pair.totalLiquidity(params.maturity) > 0, 'E507');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:614:        require(pair.totalLiquidity(params.maturity) > 0, 'E507');
Timeswap-V1-Convenience\contracts\libraries\Pay.sol:86:        if (collateralOut > 0) {
Timeswap-V1-Convenience\contracts\libraries\PayMath.sol:27:                if (due.debt > 0) {
Timeswap-V1-Convenience\contracts\libraries\SquareRoot.sol:21:        if (z % y > 0) z++;
Timeswap-V1-Convenience\contracts\libraries\Withdraw.sol:40:        if (tokensOut.asset > 0) {
Timeswap-V1-Convenience\contracts\libraries\Withdraw.sol:58:        if (tokensOut.collateral > 0) {
Timeswap-V1-Convenience\contracts\libraries\Withdraw.sol:75:        if (params.claimsIn.bond > 0)
Timeswap-V1-Convenience\contracts\libraries\Withdraw.sol:77:        if (params.claimsIn.insurance > 0)
Timeswap-V1-Core\contracts\libraries\FullMath.sol:34:                require(denominator > 0);
Timeswap-V1-Core\contracts\libraries\FullMath.sol:124:        if (mulmod(a, b, denominator) > 0) result++;
Timeswap-V1-Core\contracts\libraries\Math.sol:7:        if (x % y > 0) z++;
Timeswap-V1-Core\contracts\TimeswapPair.sol:153:        require(xIncrease > 0 && yIncrease > 0 && zIncrease > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:167:        require(liquidityOut > 0, 'E212');
Timeswap-V1-Core\contracts\TimeswapPair.sol:200:        require(liquidityIn > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:214:        if (tokensOut.asset > 0) asset.safeTransfer(assetTo, tokensOut.asset);
Timeswap-V1-Core\contracts\TimeswapPair.sol:215:        if (tokensOut.collateral > 0) collateral.safeTransfer(collateralTo, tokensOut.collateral);
Timeswap-V1-Core\contracts\TimeswapPair.sol:233:        require(xIncrease > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:236:        require(pool.state.totalLiquidity > 0, 'E206');
Timeswap-V1-Core\contracts\TimeswapPair.sol:271:        require(claimsIn.bond > 0 || claimsIn.insurance > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:289:        if (tokensOut.asset > 0) asset.safeTransfer(assetTo, tokensOut.asset);
Timeswap-V1-Core\contracts\TimeswapPair.sol:290:        if (tokensOut.collateral > 0) collateral.safeTransfer(collateralTo, tokensOut.collateral);
Timeswap-V1-Core\contracts\TimeswapPair.sol:308:        require(xDecrease > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:311:        require(pool.state.totalLiquidity > 0, 'E206');
Timeswap-V1-Core\contracts\TimeswapPair.sol:366:        if (assetIn > 0) Callback.pay(asset, assetIn, data);
Timeswap-V1-Core\contracts\TimeswapPair.sol:371:        if (collateralOut > 0) collateral.safeTransfer(to, collateralOut);

Tools Used

VS Code

Recommended Mitigation Steps

Change > 0 with != 0.

Handle

Dravee

Vulnerability details

Impact

On external functions, when using the memory keyword with a function argument, what's happening is that a memory acts as an intermediate.

Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values.

As an extract from https://ethereum.stackexchange.com/questions/74442/when-should-i-use-calldata-and-when-should-i-use-memory :

memory and calldata (as well as storage) are keywords that define the data area where a variable is stored. To answer your question directly, memory should be used when declaring variables (both function parameters as well as inside the logic of a function) that you want stored in memory (temporary), and calldata must be used when declaring an external function's dynamic parameters. The easiest way to think about the difference is that calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Proof of Concept

Timeswap-V1-Convenience\contracts\TimeswapConvenience.sol:462:    function repay(Repay memory params) external override returns (uint128 assetIn, uint128 collateralOut) {
Timeswap-V1-Convenience\contracts\TimeswapConvenience.sol:467:    function repayETHAsset(RepayETHAsset memory params)
Timeswap-V1-Convenience\contracts\TimeswapConvenience.sol:477:    function repayETHCollateral(RepayETHCollateral memory params)
Timeswap-V1-Convenience\contracts\TimeswapConvenience.sol:486:    function deployNative(Deploy memory params) external override {

Tools Used

VS Code

Recommended Mitigation Steps

Use calldata instead of memory for external functions where the function argument is read-only.

Handle

Dravee

Vulnerability details

Impact

Custom errors from Solidity 0.8.4 are cheaper than revert strings.

Proof of Concept

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

Timeswap-V1-Convenience\contracts\base\ERC20.sol:50:        require(to != address(0), 'E601');
Timeswap-V1-Convenience\contracts\base\ERC20.sol:69:        require(to != address(0), 'E601');
Timeswap-V1-Convenience\contracts\base\ERC20Permit.sol:38:        require(block.timestamp <= deadline, 'E602');
Timeswap-V1-Convenience\contracts\base\ERC20Permit.sol:45:        require(signer == owner, 'E603');
Timeswap-V1-Convenience\contracts\base\ERC721.sol:59:        require(to != owner, 'E605');
Timeswap-V1-Convenience\contracts\base\ERC721.sol:65:        require(operator != msg.sender, 'E607');
Timeswap-V1-Convenience\contracts\base\ERC721.sol:78:        require(_checkOnERC721Received(from, to, id, data), 'E608');
Timeswap-V1-Convenience\contracts\base\ERC721.sol:103:        require(to != address(0), 'E601');
Timeswap-V1-Convenience\contracts\base\ERC721.sol:104:        require(ownerOf[id] == address(0), 'E604');
Timeswap-V1-Convenience\contracts\base\ERC721.sol:117:        require(to != address(0), 'E601');
Timeswap-V1-Convenience\contracts\base\ERC721Permit.sol:32:        require(block.timestamp <= deadline, 'E602');
Timeswap-V1-Convenience\contracts\base\ERC721Permit.sol:39:        require(signer != address(0), 'E606');
Timeswap-V1-Convenience\contracts\base\ERC721Permit.sol:40:        require(signer == owner, 'E603');
Timeswap-V1-Convenience\contracts\base\ERC721Permit.sol:41:        require(spender != owner, 'E605');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:280:        require(params.debtIn > params.assetOut, 'E518');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:283:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:305:        require(dueOut.collateral <= params.maxCollateral, 'E513');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:315:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:341:        require(dueOut.debt <= params.maxDebt, 'E512');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:350:        require(params.percent <= 0x100000000, 'E505');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:353:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:375:        require(dueOut.debt <= params.maxDebt, 'E512');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:376:        require(dueOut.collateral <= params.maxCollateral, 'E513');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:385:        require(params.deadline >= block.timestamp, 'E504');
Timeswap-V1-Convenience\contracts\libraries\Borrow.sol:386:        require(params.maturity > block.timestamp, 'E508');
Timeswap-V1-Convenience\contracts\libraries\Burn.sol:77:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Burn.sol:80:        require(address(native.liquidity) != address(0), 'E502');
Timeswap-V1-Convenience\contracts\libraries\DeployNative.sol:20:        require(params.deadline >= block.timestamp, 'E504');
Timeswap-V1-Convenience\contracts\libraries\DeployNative.sol:23:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\DeployNative.sol:26:        require(address(native.liquidity) == address(0), 'E503');
Timeswap-V1-Convenience\contracts\libraries\ETH.sol:7:        require(success, 'E521');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:264:        require(params.bondOut > params.assetIn, 'E517');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:267:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:288:        require(claimsOut.insurance >= params.minInsurance, 'E515');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:298:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:324:        require(claimsOut.bond >= params.minBond, 'E514');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:333:        require(params.percent <= 0x100000000, 'E505');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:336:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:358:        require(claimsOut.bond >= params.minBond, 'E514');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:359:        require(claimsOut.insurance >= params.minInsurance, 'E515');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:368:        require(params.deadline >= block.timestamp, 'E504');
Timeswap-V1-Convenience\contracts\libraries\Lend.sol:369:        require(params.maturity > block.timestamp, 'E508');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:137:        require(params.debtIn > params.assetIn, 'E516');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:143:        require(pair.totalLiquidity(params.maturity) == 0, 'E506');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:295:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:296:        require(pair.totalLiquidity(params.maturity) > 0, 'E507');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:319:        require(liquidityOut >= params.minLiquidity, 'E511');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:320:        require(dueOut.debt <= params.maxDebt, 'E512');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:321:        require(dueOut.collateral <= params.maxCollateral, 'E513');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:454:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:455:        require(pair.totalLiquidity(params.maturity) > 0, 'E507');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:480:        require(liquidityOut >= params.minLiquidity, 'E511');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:481:        require(xIncrease <= params.maxAsset, 'E513');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:482:        require(dueOut.collateral <= params.maxCollateral, 'E512');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:613:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:614:        require(pair.totalLiquidity(params.maturity) > 0, 'E507');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:642:        require(liquidityOut >= params.minLiquidity, 'E511');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:643:        require(xIncrease <= params.maxAsset, 'E513');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:644:        require(dueOut.debt <= params.maxDebt, 'E512');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:660:        require(params.deadline >= block.timestamp, 'E504');
Timeswap-V1-Convenience\contracts\libraries\Mint.sol:661:        require(params.maturity > block.timestamp, 'E508');
Timeswap-V1-Convenience\contracts\libraries\Pay.sol:97:        require(params.deadline >= block.timestamp, 'E504');
Timeswap-V1-Convenience\contracts\libraries\Pay.sol:98:        require(params.maturity > block.timestamp, 'E508');
Timeswap-V1-Convenience\contracts\libraries\Pay.sol:101:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Pay.sol:104:        require(address(collateralizedDebt) != address(0), 'E502');
Timeswap-V1-Convenience\contracts\libraries\Withdraw.sol:70:        require(address(pair) != address(0), 'E501');
Timeswap-V1-Convenience\contracts\libraries\Withdraw.sol:73:        require(address(native.liquidity) != address(0), 'E502');
Timeswap-V1-Convenience\contracts\Bond.sol:52:        require(msg.sender == address(convenience), 'E403');
Timeswap-V1-Convenience\contracts\CollateralizedDebt.sol:45:        require(ownerOf[id] != address(0), 'E404');
Timeswap-V1-Convenience\contracts\CollateralizedDebt.sol:72:        require(msg.sender == address(convenience), 'E403');
Timeswap-V1-Convenience\contracts\CollateralizedDebt.sol:91:        require(msg.sender == address(pair), 'E401');
Timeswap-V1-Convenience\contracts\Insurance.sol:54:        require(msg.sender == address(convenience), 'E403');
Timeswap-V1-Convenience\contracts\Liquidity.sol:52:        require(msg.sender == address(convenience), 'E403');
Timeswap-V1-Convenience\contracts\TimeswapConvenience.sol:502:        require(msg.sender == address(pair), 'E701');
Timeswap-V1-Convenience\contracts\TimeswapConvenience.sol:524:        require(msg.sender == address(pair), 'E701');
Timeswap-V1-Convenience\contracts\TimeswapConvenience.sol:538:        require(msg.sender == address(pair), 'E701');
Timeswap-V1-Convenience\contracts\TimeswapConvenience.sol:558:        require(msg.sender == address(collateralizedDebt), 'E701');
Timeswap-V1-Core\contracts\libraries\BorrowMath.sol:36:        require(yIncrease >= minimum, 'E302');
Timeswap-V1-Core\contracts\libraries\Callback.sol:29:        require(_assetReserve >= assetReserve + assetIn, 'E304');
Timeswap-V1-Core\contracts\libraries\Callback.sol:30:        require(_collateralReserve >= collateralReserve + collateralIn, 'E305');
Timeswap-V1-Core\contracts\libraries\Callback.sol:41:        require(_assetReserve >= assetReserve + assetIn, 'E304');
Timeswap-V1-Core\contracts\libraries\Callback.sol:52:        require(_collateralReserve >= collateralReserve + collateralIn, 'E305');
Timeswap-V1-Core\contracts\libraries\Callback.sol:63:        require(_assetReserve >= assetReserve + assetIn, 'E304');
Timeswap-V1-Core\contracts\libraries\ConstantProduct.sol:19:        require(prod1 >= _prod1, 'E301');
Timeswap-V1-Core\contracts\libraries\ConstantProduct.sol:20:        if (prod1 == _prod1) require(prod0 >= _prod0, 'E301');
Timeswap-V1-Core\contracts\libraries\LendMath.sol:33:        require(yDecrease >= minimum, 'E302');
Timeswap-V1-Core\contracts\libraries\PayMath.sol:12:        require(uint256(assetIn) * due.collateral >= uint256(collateralOut) * due.debt, 'E303');
Timeswap-V1-Core\contracts\TimeswapFactory.sol:38:        require(_owner != address(0), 'E101');
Timeswap-V1-Core\contracts\TimeswapFactory.sol:48:        require(asset != collateral, 'E103');
Timeswap-V1-Core\contracts\TimeswapFactory.sol:49:        require(asset != IERC20(address(0)) && collateral != IERC20(address(0)), 'E101');
Timeswap-V1-Core\contracts\TimeswapFactory.sol:50:        require(getPair[asset][collateral] == IPair(address(0)), 'E104');
Timeswap-V1-Core\contracts\TimeswapFactory.sol:61:        require(msg.sender == owner, 'E102');
Timeswap-V1-Core\contracts\TimeswapFactory.sol:62:        require(_pendingOwner != address(0), 'E101');
Timeswap-V1-Core\contracts\TimeswapFactory.sol:70:        require(msg.sender == pendingOwner, 'E102');
Timeswap-V1-Core\contracts\TimeswapPair.sol:122:        require(locked == 0, 'E211');
Timeswap-V1-Core\contracts\TimeswapPair.sol:149:        require(block.timestamp < maturity, 'E202');
Timeswap-V1-Core\contracts\TimeswapPair.sol:150:        require(maturity - block.timestamp < 0x100000000, 'E208');
Timeswap-V1-Core\contracts\TimeswapPair.sol:151:        require(liquidityTo != address(0) && dueTo != address(0), 'E201');
Timeswap-V1-Core\contracts\TimeswapPair.sol:152:        require(liquidityTo != address(this) && dueTo != address(this), 'E204');
Timeswap-V1-Core\contracts\TimeswapPair.sol:153:        require(xIncrease > 0 && yIncrease > 0 && zIncrease > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:170:        require(liquidityOut > 0, 'E212');
Timeswap-V1-Core\contracts\TimeswapPair.sol:200:        require(block.timestamp >= maturity, 'E203');
Timeswap-V1-Core\contracts\TimeswapPair.sol:201:        require(assetTo != address(0) && collateralTo != address(0), 'E201');
Timeswap-V1-Core\contracts\TimeswapPair.sol:202:        require(assetTo != address(this) && collateralTo != address(this), 'E204');
Timeswap-V1-Core\contracts\TimeswapPair.sol:203:        require(liquidityIn > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:233:        require(block.timestamp < maturity, 'E202');
Timeswap-V1-Core\contracts\TimeswapPair.sol:234:        require(bondTo != address(0) && insuranceTo != address(0), 'E201');
Timeswap-V1-Core\contracts\TimeswapPair.sol:235:        require(bondTo != address(this) && insuranceTo != address(this), 'E204');
Timeswap-V1-Core\contracts\TimeswapPair.sol:236:        require(xIncrease > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:239:        require(pool.state.totalLiquidity > 0, 'E206');
Timeswap-V1-Core\contracts\TimeswapPair.sol:271:        require(block.timestamp >= maturity, 'E203');
Timeswap-V1-Core\contracts\TimeswapPair.sol:272:        require(assetTo != address(0) && collateralTo != address(0), 'E201');
Timeswap-V1-Core\contracts\TimeswapPair.sol:273:        require(assetTo != address(this) && collateralTo != address(this), 'E204');
Timeswap-V1-Core\contracts\TimeswapPair.sol:274:        require(claimsIn.bond > 0 || claimsIn.insurance > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:308:        require(block.timestamp < maturity, 'E202');
Timeswap-V1-Core\contracts\TimeswapPair.sol:309:        require(assetTo != address(0) && dueTo != address(0), 'E201');
Timeswap-V1-Core\contracts\TimeswapPair.sol:310:        require(assetTo != address(this) && dueTo != address(this), 'E204');
Timeswap-V1-Core\contracts\TimeswapPair.sol:311:        require(xDecrease > 0, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:314:        require(pool.state.totalLiquidity > 0, 'E206');
Timeswap-V1-Core\contracts\TimeswapPair.sol:350:        require(block.timestamp < maturity, 'E202');
Timeswap-V1-Core\contracts\TimeswapPair.sol:351:        require(ids.length == assetsIn.length && ids.length == collateralsOut.length, 'E205');
Timeswap-V1-Core\contracts\TimeswapPair.sol:352:        require(to != address(0), 'E201');
Timeswap-V1-Core\contracts\TimeswapPair.sol:353:        require(to != address(this), 'E204');
Timeswap-V1-Core\contracts\TimeswapPair.sol:361:            require(due.startBlock != BlockNumber.get(), 'E207');
Timeswap-V1-Core\contracts\TimeswapPair.sol:362:            if (owner != msg.sender) require(collateralsOut[i] == 0, 'E213');

Tools Used

VS Code

Recommended Mitigation Steps

Replace revert strings with custom errors.

Handle

Dravee

Vulnerability details

Impact

Due to how constant variables are implemented, an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232

Proof of Concept

Timeswap-V1-Convenience\contracts\libraries\DateTime.sol:31:    uint constant SECONDS_PER_DAY = 24 * 60 * 60;
Timeswap-V1-Convenience\contracts\libraries\DateTime.sol:32:    uint constant SECONDS_PER_HOUR = 60 * 60;

Tools Used

VS Code

Recommended Mitigation Steps

Change these expressions from constant to immutable and implement the calculation in the constructor

Handle

Dravee

Vulnerability details

Impact

Some of the require statements can be placed earlier to reduce gas usage on revert. As a reminder from the Ethereum Yellow Paper, Appendix G and Appendix H:

• TIMESTAMP costs 2 gas
• ADDRESS costs 2 gas
• MLOAD costs 3 gas

Proof of Concept

The following can be reorder to save gas on revert:

        require(block.timestamp < maturity, 'E202'); 
        require(ids.length == assetsIn.length && ids.length == collateralsOut.length, 'E205');
        require(to != address(0), 'E201');
        require(to != address(this), 'E204');

to

        require(block.timestamp < maturity, 'E202');
        require(to != address(0), 'E201');
        require(to != address(this), 'E204');
        require(ids.length == assetsIn.length && ids.length == collateralsOut.length, 'E205');

Tools Used

VS Code

Recommended Mitigation Steps

Relocate the said require statements

Handle

Dravee

Vulnerability details

Impact

Removing unused named return variables can reduce gas usage and improve code clarity.

Proof of Concept

In TimeswapPair:constantProduct(), you return using both named return and actual return statement. To save gas and improve code quality consider using only one of those.

    /// @inheritdoc IPair
    function constantProduct(uint256 maturity)
        external
        view
        override
        returns (
            uint112 x,
            uint112 y,
            uint112 z
        )
    {
        State memory state = pools[maturity].state;
        return (state.x, state.y, state.z);
    }

Tools Used

VS Code

Recommended Mitigation Steps

Remove the unused named returns

Handle

Dravee

Vulnerability details

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

Instances include:

Timeswap-V1-Convenience\contracts\libraries\NFTTokenURIScaffold.sol:119:        for(uint i = 0; i < lengthDiff; i++) {
Timeswap-V1-Convenience\contracts\libraries\NFTTokenURIScaffold.sol:147:        for(uint i = 0; i < lengthDiff; i++) {
Timeswap-V1-Convenience\contracts\libraries\NFTTokenURIScaffold.sol:201:        for (uint256 i = 0; i < data.length; i++) {

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove explicit initialization for default values.

Handle

Dravee

Vulnerability details

Impact

As mistakes happen, address(0) checks should be made to avoid having to redeploy contracts

Proof of Concept

Here, the variable weth can't be updated after deployment: https://github.com/code-423n4/2022-01-timeswap/blob/5960e07d39f2b4a60cfabde1bd51f4b1e62e7e85/Timeswap/Timeswap-V1-Convenience/contracts/TimeswapConvenience.sol#L64

Tools Used

VS Code

Recommended Mitigation Steps

Add the address(0) check

Handle

Dravee

Vulnerability details

Impact

The acceptOwner() external function can be called indefinitely instead of only once. The contract's state doesn't reflect reality. The code doesn't follow the standard implementation of a 2-step ownership transfer.

Proof of Concept

Here's the current acceptOwner() external function, which lacks a reset of pendingOwner to address(0) :

    function acceptOwner() external override {
        require(msg.sender == pendingOwner, 'E102');
        owner = msg.sender;

        emit AcceptOwner(msg.sender);
    }

Tools Used

VS Code

Recommended Mitigation Steps

Change the code to:

    function acceptOwner() external override {
        require(msg.sender == pendingOwner, 'E102');
        owner = msg.sender;
        pendingOwner = address(0); // @audit : line to add

        emit AcceptOwner(msg.sender);
    }