Platform: Code4rena
Start Date: 13/12/2021
Pot Size: $75,000 USDC
Total HM: 11
Participants: 30
Period: 7 days
Judge: leastwood
Total Solo HM: 4
Id: 68
League: ETH
Rank: 5/30
Findings: 4
Award: $4,412.08
🌟 Selected for report: 8
🚀 Solo Findings: 0
cmichel
The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success.
Some tokens do not revert if the transfer failed but return false instead.
See:
SingleNativeTokenExitV2.exit's outputToken.transfer(msg.sender, outputTokenBalance);PieFactoryContract.bakePie's pie.transfer(msg.sender, _initialSupply);Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and the tokens remain in the SingleNativeTokenExitV2 contract and could potentially be stolen by someone else.
We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.
#0 - loki-sama
2022-01-04T10:29:21Z
duplicate #232
#1 - 0xleastwood
2022-01-22T04:08:21Z
Marking as primary issue.
#2 - 0xleastwood
2022-01-22T04:09:48Z
Nice find! I think this is valid considering the extent basket tokens are used. It is more than likely that non-standard tokens will be utilised.
cmichel
The SingleNativeTokenExitV2.exit function performs a list of arbitrary user-defined swaps on the exited token basket.
These could result in many different final "output" tokens ending up in the contract after the swaps.
However, the contract assumes that there's only a single outputToken, more specifically, the last path token of the last swap of the first trade:
function exit(ExitTokenStructV2 calldata _exitTokenStruct) external { _exit(_exitTokenStruct); address[] calldata path = _exitTokenStruct .trades[0] .swaps[_exitTokenStruct.trades[0].swaps.length - 1] .path; // @audit why only pay out the last path token of the last swap of the first trade? non-intuitive, should be documented IERC20 outputToken = IERC20(path[path.length - 1]); //this could be not the target token // ... }
The single output token that is decided to be paid out seems quite arbitrary and it's non-intuitive and not documented for any caller trying to exit their position through this contract.
Iterate through all the tokens of the basket and pay out all of them if there's a balance in the contract after the trades.
#0 - loki-sama
2022-01-04T10:28:14Z
The swaps should all be going to one token so this could be improved
#1 - loki-sama
2022-01-05T17:32:58Z
duplicate #176
🌟 Selected for report: cmichel
1001.4677 USDC - $1,001.47
cmichel
The rebalance manager can perform trades on the entire basket amount and steal tokens this way, for example, by adding their own contract as the exchange through setExchange, followed by approving tokens to this contract through the RebalanceManagerV2.rebalance action.
The rebalance manager of each basket needs to be trusted.
Document that the rebalance manager needs to be trusted as well, the current docs talk about an owner but it was not clear to me that the rebalance manager is always assumed to be the owner of a basket.
- RebalanceMangers - Enable trading the baskets underlying token to rebalance the index constitution (only owner).
#0 - loki-sama
2022-01-04T10:16:09Z
This is intended behavior
#1 - 0xleastwood
2022-01-22T03:17:14Z
I think the warden has outlined missing documentation related to the trusted setup of the rebalance manager. As such, I'll mark this as low considering this is intended behaviour.
cmichel
The PieFactoryContract.addFacet function does not check if the facet has already been added.
The same facet can be added several times and the defaultCut can include duplicate facets that will not be removed by a single removeFacet call.
Check that facet is not in defaultCut already when adding it through addFacet.
cmichel
The address.transfer function is used to send ETH to an account.
It is restricted to a low amount of GAS and might fail if GAS costs change in the future or if a smart contract's fallback function handler implements anything non-trivial.
See:
SingleNativeTokenExit.exitEthSingleNativeTokenExitV2.exitEthEthSingleTokenJoin.joinTokenEthEthSingleTokenJoinV2.joinTokenEthConsider using the lower-level .call{value: value} instead and check its success return value.
#0 - loki-sama
2022-01-03T12:21:53Z
duplicate #175
cmichel
The BasketFacet.calcOutStandingAnnualizedFee function calculates the pending fee as:
// @audit reorder div to end to not lose precision? totalSupply.mul(annualizedFee).div(10**18).mul(timePassed).div( 365 days );
The early division by 1e18 will lead to a loss of precision. Slightly fewer fees will be collected.
Generally, it's recommended to first do all multiplications before doing the divisions to not lose precision due to integer divisions.
#0 - 0xleastwood
2022-01-23T05:06:43Z
Duplicate of #155
cmichel
The initialize function that initializes important contract state can be called by anyone.
See:
MintableERC20.initializePolygonERC20Wrapper.initializeThe attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract. In the best case for the victim, they notice it and have to redeploy their contract costing gas.
Use the constructor to initialize non-proxied contracts.
For initializing proxy contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.
#0 - 0xleastwood
2022-01-23T05:06:01Z
As per deployment script, I think this is valid.
cmichel
The EthSingleTokenJoin contract (and the other similar join contracts) allow any _INTERMEDIATE_TOKEN to be set by the constructor but the code only works if it's the wrapped native token (WETH or equivalent for other chains) due to the calls to .withdraw and the fallback function which is assumed to deposit.
The contract will not fail early if a wrong INTERMEDIATE_TOKEN is set, it'll only be noticeable later when the actual joinTokenEth function is called.
Hardcode the wrapped native token address based on the chain id.
🌟 Selected for report: cmichel
cmichel
The SingleTokenJoin._joinTokenSingle trades a single input token with a predetermined input amount to all basket tokens (according to the individual token weights in the basket).
The basket share output amount is defined as a parameter, as well as the input amount, however, these two values are independent.
It can easily happen that the input amount that needs to be traded to reach the specified basket share output amount is too low as the price of any token on the DEX has changed since the transaction was submitted.
Don't specify the input amount at all, work backwards from the desired shares output amount to the token amounts, from these token amounts to the sum of the intermediate token, from this intermediate token to the initial input token amount.
This can be done using the Uniswap getAmountsIn.
🌟 Selected for report: cmichel
78.3316 USDC - $78.33
cmichel
The SingleTokenJoin._joinTokenSingle function computes an intermediate maxAmountIn value of amountsIn[0]:
function _joinTokenSingle(JoinTokenStruct calldata _joinTokenStruct) internal { // ... swaps to intermediate first path[0] = address(INTERMEDIATE_TOKEN); _maxApprove(INTERMEDIATE_TOKEN, address(uniSwapLikeRouter)); for (uint256 i; i < tokens.length; i++) { path[1] = tokens[i]; uint256[] memory amountsIn = uniSwapLikeRouter.getAmountsIn( amounts[i], path ); uniSwapLikeRouter.swapTokensForExactTokens( amounts[i], amountsIn[0], // @audit gas: just use MAX maxAmountIn path, address(this), _joinTokenStruct.deadline ); _maxApprove(IERC20(tokens[i]), _joinTokenStruct.outputBasket); } IBasketFacet(_joinTokenStruct.outputBasket).joinPool( _joinTokenStruct.outputAmount, _joinTokenStruct.referral ); // ######## SEND TOKEN ######### uint256 outputAmount = outputToken.balanceOf(address(this)); require( outputAmount == _joinTokenStruct.outputAmount, "FAILED_OUTPUT_AMOUNT" ); outputToken.safeTransfer(msg.sender, outputAmount); }
It's not required to compute the input amount that is needed for the exact output swap, just do the swap and use a maxAmountIn of type(uint256).max.
The final min return check on the entire amount (outputAmount == _joinTokenStruct.outputAmount) is enough to guarantee no sandwiching.
cmichel
The SingleTokenJoin._joinTokenSingle function computes an intermediate amountOutMin value of amountsOut[amountsOut.length - 1]:
function _joinTokenSingle(JoinTokenStruct calldata _joinTokenStruct) internal { // ######## INIT TOKEN ######### IERC20 inputToken = IERC20(_joinTokenStruct.inputToken); IERC20 outputToken = IERC20(_joinTokenStruct.outputBasket); (address[] memory tokens, uint256[] memory amounts) = IBasketFacet( _joinTokenStruct.outputBasket ).calcTokensForAmount(_joinTokenStruct.outputAmount); // ######## SWAP TOKEN ######### address[] memory path = new address[](2); if (_joinTokenStruct.inputToken != address(INTERMEDIATE_TOKEN)) { _maxApprove(inputToken, address(uniSwapLikeRouter)); path[0] = _joinTokenStruct.inputToken; path[1] = address(INTERMEDIATE_TOKEN); uint256[] memory amountsOut = uniSwapLikeRouter.getAmountsOut( _joinTokenStruct.inputAmount, path ); uniSwapLikeRouter.swapExactTokensForTokens( _joinTokenStruct.inputAmount, amountsOut[amountsOut.length - 1], path, address(this), _joinTokenStruct.deadline ); } path[0] = address(INTERMEDIATE_TOKEN); _maxApprove(INTERMEDIATE_TOKEN, address(uniSwapLikeRouter)); for (uint256 i; i < tokens.length; i++) { path[1] = tokens[i]; uint256[] memory amountsIn = uniSwapLikeRouter.getAmountsIn( amounts[i], path ); uniSwapLikeRouter.swapTokensForExactTokens( amounts[i], amountsIn[0], path, address(this), _joinTokenStruct.deadline ); _maxApprove(IERC20(tokens[i]), _joinTokenStruct.outputBasket); } IBasketFacet(_joinTokenStruct.outputBasket).joinPool( _joinTokenStruct.outputAmount, _joinTokenStruct.referral ); // ######## SEND TOKEN ######### uint256 outputAmount = outputToken.balanceOf(address(this)); require( outputAmount == _joinTokenStruct.outputAmount, "FAILED_OUTPUT_AMOUNT" ); outputToken.safeTransfer(msg.sender, outputAmount); }
This trade just swaps the input token to the intermediate token and as different amounts are used for the subsequent swaps, the entire uniSwapLikeRouter.getAmountsOut(_joinTokenStruct.inputAmount, path) calculation is not needed.
The final min return check on the entire amount (outputAmount == _joinTokenStruct.outputAmount) is enough to guarantee no sandwiching.