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: 16/30
Findings: 3
Award: $840.09
🌟 Selected for report: 4
🚀 Solo Findings: 0
228.0947 USDC - $228.09
pmerkleplant
There's a griefing attack vulnerability in the function joinTokenSingle in
SingleTokenJoin.sol as well as SingleTokenJoinV2.sol which makes any user
transaction fail with "FAILED_OUTPUT_AMOUNT".
The JoinTokenStruct argument for joinTokenSingle includes a field outputAmount
to indicate the amount of tokens the user should receive after joining a basket
(see line 135 and 130).
However, this amount is compared to the contract's balance of the token and reverts if the amount is unequal.
If an attacker sends some amount of a basket's token to the contract, every call to this function will fail as long as the output token equals the attacker's token send.
Refactor the require statement to expect at least the outputAmount of tokens,
i.e. require(outputAmount >= _joinTokenStruct.outputAmount).
🌟 Selected for report: pmerkleplant
Also found by: hyh
pmerkleplant
The comment for the variable BasketStorage.exitFeeBeneficiaryShare states that
the fee goes to the pool (see line 18 in LibBasketStorage.sol).
However, the fee is not send to the pool itself, but rather to the BasketStorage.feeBeneficiary (see line 179 in BasketFacet.sol) which can be set to an arbitrary address.
Change the comment to reflect that the fee goes to the feeBeneficiary, not the pool.
pmerkleplant
Caching the array length outside a loop in a variable saves reading it on each iteration as long as the array's length is not changed during the loop.
Therefore, the following loops could be refactored:
./callManagers/RebalanceManagerV2.sol:155: for (uint256 i; i < _swapsV2.length; i++) ./factories/PieFactoryContract.sol:88: for (uint256 i = 0; i < _tokens.length; i++) ./callManagers/RebalanceManagerV3.sol:166: for (uint256 i; i < _swapsV2.length; i++) ./callManagers/RebalanceManagerV3.sol:171: for (uint256 j; j < trade.swaps.length; j++) ./facets/Call/CallFacet.sol:55: for (uint256 i = 0; i < callStorage.callers.length; i++) ./facets/Call/CallFacet.sol:82: for (uint256 i = 0; i < _targets.length; i++) ./facets/Call/CallFacet.sol:95: for (uint256 i = 0; i < _targets.length; i++) ./singleJoinExit/SingleNativeTokenExitV2.sol:74: for (uint256 i; i < _exitTokenStruct.trades.length; i++) ./singleJoinExit/SingleNativeTokenExitV2.sol:76: for (uint256 j; j < trade.swaps.length; j++) ./singleJoinExit/SingleTokenJoin.sol:108: for (uint256 i; i < tokens.length; i++) ./singleJoinExit/SingleTokenJoinV2.sol:86: for (uint256 i; i < _joinTokenStruct.trades.length; i++) ./singleJoinExit/SingleTokenJoinV2.sol:91: for (uint256 j; j < trade.swaps.length; j++) ./singleJoinExit/SingleTokenJoinV2.sol:100: for (uint256 j; j < trade.swaps.length; j++) ./singleJoinExit/SingleTokenJoinV2.sol:117: for (uint256 i; i < tokens.length; i++) ./callManagers/RebalanceManager.sol:218: for (uint256 i; i < _swapsV2.length; i++) ./callManagers/RebalanceManager.sol:234: for (uint256 i; i < _swapsV3.length; i++) ./singleJoinExit/SingleNativeTokenExit.sol:69: for (uint256 i; i < tokens.length; i++) ./facets/Basket/BasketFacet.sol:50: for (uint256 i; i < bs.tokens.length; i++) (changes the array size, but breaks afterwards) ./facets/Basket/BasketFacet.sol:160: for (uint256 i; i < bs.tokens.length; i++) ./facets/Basket/BasketFacet.sol:202: for (uint256 i; i < bs.tokens.length; i++) ./facets/Basket/BasketFacet.sol:321: for (uint256 i = 0; i < tokens.length; i++) ./facets/Basket/BasketFacet.sol:348: for (uint256 i; i < bs.tokens.length; i++) ./facets/Basket/BasketFacet.sol:381: for (uint256 i; i < bs.tokens.length; i++)
Use the following pattern for refactoring:
uint length = arr.length; for (uint i; i < length; i++) { // ... }
grep
#0 - 0xleastwood
2022-01-23T22:18:15Z
Duplicate of #249
🌟 Selected for report: pmerkleplant
pmerkleplant
The subtraction in ERC20Facet::decreaseApproval in line 128 could be unchecked, i.e. it's not necessary to use OZ's SafeMath::sub function here.
An underflow is not possible, as the expression is only called if oldValue >= _amount.
Use solidity's builtin subtraction instead of OZ's SafeMath::sub to save gas
on unnecessary checks.
🌟 Selected for report: pmerkleplant
pmerkleplant
If a variable is not initialized, it is assumed to have the default value. Explicitly initializing a variable with its default value costs unnecessary gas.
For more info, see Mudit Gupta's Blog at point "No need to initialize variables with default values".
Therefore, following variable declarations could be refactored:
./factories/PieFactoryContract.sol:88: for (uint256 i = 0; i < _tokens.length; i++) ./facets/Call/CallFacet.sol:55: for (uint256 i = 0; i < callStorage.callers.length; i++) ./facets/Call/CallFacet.sol:82: for (uint256 i = 0; i < _targets.length; i++) ./facets/Call/CallFacet.sol:95: for (uint256 i = 0; i < _targets.length; i++) ./facets/Basket/BasketFacet.sol:321: for (uint256 i = 0; i < tokens.length; i++)
grep