Amun contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

.transfer usage is discouraged and is no longer recommended as recipients with custom fallback functions (smart contracts) will not be able to handle that. You can read more here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Recommended Mitigation Steps

Solution (don't forget re-entrancy protection): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L53-L59

Handle

pauliax

Vulnerability details

Impact

Rebalance managers approve max if allowance < quantity. SingleTokenJoin, SingleTokenJoinV2, SingleNativeTokenExit, and SingleNativeTokenExitV2. approve max if allowance < balance.

The problem is that some tokens, e.g. USDT require resetting approval to 0 before approving any value again.

Recommended Mitigation Steps

Consider using this approach:

  token.approve(spender, 0);
  token.approve(spender, uint256(-1));

Handle

pauliax

Vulnerability details

Impact

I am sorry if I incorrectly understood the intentions of contract PolygonERC20Wrapper but the comments and the code do not align here:

Function deposit: "Should handle deposit by minting the required amount for user. Make sure minting is done only by this function". However, the code does not perform any minting and just transfers the underlying tokens to the user.

On the other hand, functions withdraw and withdrawTo state: "Should burn user's tokens" but it actually performs both, minting and burning:

  _mint(recipient, amount);
  _burn(recipient, amount);

Recommended Mitigation Steps

I was late to verify this with the sponsor, so make sure this is the intended behavior, and update comments to match the codebase.

Handle

pauliax

Vulnerability details

Impact

Assigned operations to constant variables are re-evaluated every time:

  bytes32 constant BASKET_STORAGE_POSITION =
        keccak256("diamond.standard.basket.storage");
  bytes32 constant CALL_STORAGE_POSITION =
        keccak256("diamond.standard.call.storage");
  bytes32 constant REENTRY_STORAGE_POSITION =
        keccak256("diamond.standard.reentry.storage");
  bytes32 constant ERC_20_STORAGE_POSITION =
        keccak256(
            // Compatible with pie-smart-pools
            "PCToken.storage.location"
        );

See https://github.com/ethereum/solidity/issues/9232

Change from 'constant' to 'immutable'.

Handle

pauliax

Vulnerability details

Impact

_amount.add(feeAmount) is re-evaluated again and again inside the loop:

  for (uint256 i; i < bs.tokens.length; i++) {
     ...
      uint256 tokenAmount =
          balance(address(token)).mul(_amount.add(feeAmount)).div(
              totalSupply
          );
     ...
  }

These values do not change so consider extracting outside the loop to reduce gas costs.

Handle

pauliax

Vulnerability details

Impact

Contracts SingleTokenJoinV2 and SingleNativeTokenExitV2 initialize uniSwapLikeRouter, but never actually use it, as swap.exchange is used instead. So it basically trusts the user input. Consider removing uniSwapLikeRouter if that was the intention to save some gas.

Handle

pauliax

Vulnerability details

Impact

structs can be optimized, e.g. inputAmount and outputAmount could be stored in uint112, as that is what Uniswap like exchanges use: https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L22-L23

'deadline' also usually fits in a reasonable limit of at least uint32 or uint64. 'entryFee', 'exitFee', etc are also capped, so you should consider revisiting structs to find optimal sizes and orders of the variables.

Handle

pauliax

Vulnerability details

Impact

Not used state variables:

  bool private recovered;
  bool private burned;

Consider removing them to reduce deployment costs.