Amun contest - robee's results

Handle

robee

Vulnerability details

The use of transfer()/ send() to send ETH may have unintended outcomes on the eth being sent to the receiver. Eth may be irretrievable or undelivered if the msg.sender or feeRecipient is a smart contract. Funds can potentially be lost if; 1. The smart contract fails to implement the payable fallback function 2. The fallback function uses more than 2300 gas units The latter situation may occur in the instance of gas cost changes. The impact would mean that any contracts receiving funds would potentially be unable to retrieve funds from the transaction. A detailed explanation of why relying on payable().transfer() may result in unexpected loss of eth can be found here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now This is not just a best-practice advice since the return value isn't considered!!! If you would consider it then it was just a best practice.

    EthSingleTokenJoin.sol, 36, msg.sender.transfer(remainingIntermediateBalance); 
    EthSingleTokenJoinV2.sol, 36, msg.sender.transfer(remainingIntermediateBalance); 
    SingleNativeTokenExit.sol, 92, msg.sender.transfer(intermediateTokenBalance); 
    SingleNativeTokenExitV2.sol, 120, msg.sender.transfer(intermediateTokenBalance); 

Handle

robee

Vulnerability details

Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value. We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens. The list of occurrences in format (solidity file, line number, actual line) RebalanceManager.sol, 83, IERC20(swap.tokenIn).approve.selector, RebalanceManager.sol, 130, IERC20(swap.path[0]).approve.selector, RebalanceManagerV2.sol, 69, IERC20(swap.path[0]).approve.selector, RebalanceManagerV3.sol, 74, IERC20(path[0]).approve.selector, SingleNativeTokenExit.sol, 43, token.approve(spender, uint256(-1)); SingleNativeTokenExitV2.sol, 54, token.approve(spender, uint256(-1)); SingleTokenJoin.sol, 42, token.approve(spender, uint256(-1)); SingleTokenJoinV2.sol, 52, token.approve(spender, uint256(-1));

Handle

robee

Vulnerability details

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value.They must first be approved by zero and then the actual allowance must be approved. You don't first approve 0 in the following places in the codebase:

    approve without approving 0 first SingleNativeTokenExit.sol, 43, token.approve(spender, uint256(-1)); 
    approve without approving 0 first SingleNativeTokenExitV2.sol, 54, token.approve(spender, uint256(-1)); 
    approve without approving 0 first SingleTokenJoin.sol, 42, token.approve(spender, uint256(-1)); 
    approve without approving 0 first SingleTokenJoinV2.sol, 52, token.approve(spender, uint256(-1)); 

Handle

robee

Vulnerability details

You should use safe math for solidity version <8 since there is no default over/under flow check it suchversions of solidity. The contract RebalanceManager.sol doesn't use safe math and is of solidity version < 8 The contract RebalanceManagerV2.sol doesn't use safe math and is of solidity version < 8 The contract RebalanceManagerV3.sol doesn't use safe math and is of solidity version < 8 The contract CallFacet.sol doesn't use safe math and is of solidity version < 8 The contract CallProtection.sol doesn't use safe math and is of solidity version < 8 The contract ReentryProtection.sol doesn't use safe math and is of solidity version < 8 The contract PieFactoryContract.sol doesn't use safe math and is of solidity version < 8 The contract Imports.sol doesn't use safe math and is of solidity version < 8 The contract IBasketFacet.sol doesn't use safe math and is of solidity version < 8 The contract ICallFacet.sol doesn't use safe math and is of solidity version < 8 The contract IERC20Facet.sol doesn't use safe math and is of solidity version < 8 The contract IExperiPie.sol doesn't use safe math and is of solidity version < 8 The contract IPangolinRebalanceManager.sol doesn't use safe math and is of solidity version < 8 The contract IRebalanceManager.sol doesn't use safe math and is of solidity version < 8 The contract IRebalanceManagerV2.sol doesn't use safe math and is of solidity version < 8 The contract IRebalanceManagerV3.sol doesn't use safe math and is of solidity version < 8 The contract IUniswapV2Factory.sol doesn't use safe math and is of solidity version < 8 The contract IUniswapV2Pair.sol doesn't use safe math and is of solidity version < 8 The contract IWrappedNativeToken.sol doesn't use safe math and is of solidity version < 8 The contract EthSingleTokenJoin.sol doesn't use safe math and is of solidity version < 8 The contract EthSingleTokenJoinV2.sol doesn't use safe math and is of solidity version < 8

Handle

robee

Vulnerability details

This issue is about arithmetic computation that could have been done more percise. The following are places in the codebase in which you multiplied after the divisions. Doing the multiplications at start lead to more accurate calculations. This is a list of places in the code that this appears (Solidity file, line number, actual line):

    BasketFacet.sol, 259, totalSupply.mul(annualizedFee).div(10**18).mul(timePassed).div( ...

Handle

robee

Vulnerability details

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: RebalanceManager.sol, i, 218
    change to prefix increment and unchecked: RebalanceManager.sol, i, 234
    change to prefix increment and unchecked: RebalanceManagerV2.sol, i, 155
    change to prefix increment and unchecked: RebalanceManagerV3.sol, i, 166
    change to prefix increment and unchecked: BasketFacet.sol, i, 50
    change to prefix increment and unchecked: BasketFacet.sol, i, 160
    change to prefix increment and unchecked: BasketFacet.sol, i, 202
    change to prefix increment and unchecked: BasketFacet.sol, i, 321
    change to prefix increment and unchecked: BasketFacet.sol, i, 348
    change to prefix increment and unchecked: BasketFacet.sol, i, 381
    change to prefix increment and unchecked: CallFacet.sol, i, 55
    change to prefix increment and unchecked: CallFacet.sol, i, 82
    change to prefix increment and unchecked: CallFacet.sol, i, 95
    change to prefix increment and unchecked: PieFactoryContract.sol, i, 88
    change to prefix increment and unchecked: SingleNativeTokenExit.sol, i, 69
    change to prefix increment and unchecked: SingleNativeTokenExitV2.sol, i, 74
    change to prefix increment and unchecked: SingleTokenJoin.sol, i, 108
    change to prefix increment and unchecked: SingleTokenJoinV2.sol, i, 86
    change to prefix increment and unchecked: SingleTokenJoinV2.sol, i, 117

Handle

robee

Vulnerability details

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

    RebalanceManager.sol Variable basket is read 2 times in the function: rebalance
    RebalanceManagerV3.sol Variable basket is read 2 times in the function: rebalance
    BasketFacet.sol Variable MIN_AMOUNT is read 2 times in the function: exitPool
    PieFactoryContract.sol Variable defaultController is read 2 times in the function: bakePie
    SingleTokenJoin.sol Variable uniSwapLikeRouter is read 2 times in the function: _joinTokenSingle
    SingleTokenJoin.sol Variable INTERMEDIATE_TOKEN is read 3 times in the function: _joinTokenSingle

Handle

robee

Vulnerability details

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    MintableERC20.sol, initialize, 12
    PolygonERC20Wrapper.sol, initialize, 16

Handle

robee

Vulnerability details

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory We recommend to change from: for (uint256 i=0; i<array.length; i++) { ... } to: uint len = array.length for (uint256 i=0; i<len; i++) { ... } These functions use not using prefix increments (++x) or not using the unchecked keyword:

    RebalanceManager.sol, _swapsV2, 218
    RebalanceManager.sol, _swapsV3, 234
    RebalanceManagerV2.sol, _swapsV2, 155
    RebalanceManagerV3.sol, _swapsV2, 166
    BasketFacet.sol, tokens.bs, 50
    BasketFacet.sol, tokens.bs, 160
    BasketFacet.sol, tokens.bs, 202
    BasketFacet.sol, tokens, 321
    BasketFacet.sol, tokens.bs, 348
    BasketFacet.sol, tokens.bs, 381
    CallFacet.sol, callers.callStorage, 55
    CallFacet.sol, _targets, 82
    CallFacet.sol, _targets, 95
    PieFactoryContract.sol, _tokens, 88
    SingleNativeTokenExit.sol, tokens, 69
    SingleNativeTokenExitV2.sol, trades._exitTokenStruct, 74
    SingleTokenJoin.sol, tokens, 108
    SingleTokenJoinV2.sol, trades._joinTokenStruct, 86
    SingleTokenJoinV2.sol, tokens, 117

Handle

robee

Vulnerability details

In the following files there are state variables that could be set immutable to save gas. The list of format <solidity file>, <state variable name that could be immutable>:

    basket in RebalanceManagerV2.sol
    uniSwapLikeRouter in SingleNativeTokenExit.sol
    INTERMEDIATE_TOKEN in SingleNativeTokenExit.sol
    uniSwapLikeRouter in SingleNativeTokenExitV2.sol
    INTERMEDIATE_TOKEN in SingleNativeTokenExitV2.sol
    uniSwapLikeRouter in SingleTokenJoin.sol
    INTERMEDIATE_TOKEN in SingleTokenJoin.sol
    uniSwapLikeRouter in SingleTokenJoinV2.sol
    INTERMEDIATE_TOKEN in SingleTokenJoinV2.sol
    predicateProxy in MintableERC20.sol
    underlying in PolygonERC20Wrapper.sol
    childChainManager in PolygonERC20Wrapper.sol

Handle

robee

Vulnerability details

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: PieFactoryContract.sol, In line 79, Require message length to shorten: 35, The message: CANNOT_CREATE_ZERO_TOKEN_LENGTH_PIE
    Solidity file: MintableERC20.sol, In line 17, Require message length to shorten: 38, The message: new predicateProxy is the zero address
    Solidity file: PolygonERC20Wrapper.sol, In line 22, Require message length to shorten: 39, The message: new underlyingToken is the zero address

Handle

robee

Vulnerability details

In the following files there are contract imports that aren't used. Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore). The following is a full list of all unused imports, we went through the whole code to find it :) <solidity file> <line number> <actual import line>:

    RebalanceManager.sol, line 7, import "@uniswap/v3-periphery/contracts/interfaces/IQuoterV2.sol";
    CallFacet.sol, line 7, import "../shared/Access/CallProtection.sol";
    LibERC20.sol, line 4, import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
    IPangolinRebalanceManager.sol, line 4, import {IPangolinRouter} from "@pangolindex/exchange-contracts/contracts/pangolin-periphery/interfaces/IPangolinRouter.sol";
    IPangolinRebalanceManager.sol, line 5, import "./IExperiPie.sol";
    MintableERC20.sol, line 3, import {ERC20Upgradeable, ERC20PermitUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/draft-ERC20PermitUpgradeable.sol";

Handle

robee

Vulnerability details

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    BasketFacet.sol, 321
    CallFacet.sol, 55
    CallFacet.sol, 82
    CallFacet.sol, 95
    PieFactoryContract.sol, 88

Handle

robee

Vulnerability details

There are places in the code (especially in for-each loops) that loads the same array element more than once. In such cases, only one array boundaries check should take place, and the rest are unnecessary. Therefore, this array element should be cached in a local variable and then be loaded again using this local variable, skipping the redundent second array boundaries check:

    PieFactoryContract.sol, variable name: _tokens times: 2 at: bakePie
    SingleNativeTokenExit.sol, variable name: tokens times: 2 at: exitEth
    SingleTokenJoin.sol, variable name: tokens times: 2 at: _joinTokenSingle
    SingleTokenJoin.sol, variable name: amounts times: 2 at: _joinTokenSingle
    SingleTokenJoinV2.sol, variable name: inputs times: 4 at: _joinTokenSingle

Handle

robee

Vulnerability details

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    The function chargeOutstandingAnnualizedFee in BasketFacet.sol could be set external
    The function calcOutStandingAnnualizedFee in BasketFacet.sol could be set external
    The function balance in BasketFacet.sol could be set external
    The function callNoValue in CallFacet.sol could be set external
    The function call in CallFacet.sol could be set external
    The function initialize in MintableERC20.sol could be set external
    The function initialize in PolygonERC20Wrapper.sol could be set external