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: 3/30
Findings: 4
Award: $8,587.28
🌟 Selected for report: 4
🚀 Solo Findings: 1
certora
https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleTokenJoin.sol#L135
the balance of outputToken is checked to be exactly _joinTokenStruct.outputAmount.
It is not recommeded and it's better to use >=
The worst scenario is a denial of service in case there is already an amount of the output token in the contract. It is easily achieved by transferring tokens to the contract. then it will lead to denial of service of joinTokenSingle because the actual balance at the end of the function will be greater than _joinTokenStruct.outputAmount so the transaction would revert.
An attacker can transfer basket tokens to SingleTokenJoin contract, after that, no one will be able to use it.
Let's assume the attacker transferred one basket token, then a users tries to use joinTokenSingle.
in the following line, the basket will send _joinTokenStruct.outputAmount to address(this):
https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L122
after that, outputToken.balanceOf(address(this)) will be _joinTokenStruct.outputAmount + 1. because the attacker transferred one token.
therefore the require statement will fail and the transaction will revert:
uint256 outputAmount = outputToken.balanceOf(address(this)); require( outputAmount == _joinTokenStruct.outputAmount, "FAILED_OUTPUT_AMOUNT" );
this can never be changed and the contract will stay in a state of denial of service forever.
change to
require( outputAmount >= _joinTokenStruct.outputAmount, "FAILED_OUTPUT_AMOUNT" );
This issue is also in: https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L130
#0 - loki-sama
2022-01-03T09:28:52Z
duplicate #81
certora
there is no check on the return value of call.
(bool success, ) = address(INTERMEDIATE_TOKEN).call{value: msg.value}(""); require(success);
#0 - 0xleastwood
2022-01-23T04:57:13Z
Duplicate of #237
🌟 Selected for report: certora
3004.4031 USDC - $3,004.40
certora
after that fee is calculated, it is minted to the feeBeneficiary. simply minting the exact amount results lower fee than it should be.
feeBeneficiary will get less fees than it should.
let's assume that the basket assets are worth 1M dollars, and totalSupply = 1M.
the result of calcOutStandingAnnualizedFee is 100,00 so the feeBeneficiary should get 100,00 dollars.
however, when minting 100,00 the totalSupply will increase to 1,100,000 so they will own 100000/1100000* (1M dollars) = 90909.09 dollars instead of 100k
#0 - loki-sama
2022-01-04T10:53:29Z
This is mitigated by the feeBeneficiary diluting his own shares if he gets fees on his fees.
#1 - 0xleastwood
2022-01-23T02:07:11Z
I'm not exactly sure if I understand what the warden is stating here. Could you confirm @loki-sama ?
#2 - loki-sama
2022-01-24T14:08:36Z
Ok, I myself misunderstood. He is correct that we don't get the full value. When we take a fee of 10% like from his example. What we do is mint 10% of the basket to ourselves. That 10% after minting is not holding 10% of the underling.
certora
input = IERC20(targetToken).balanceOf(address(basket)) - oldBalance;
input is calculated with regular substract, not sub, and it can lead to underflow. An underflow can occur if the balance after the swap is less than the balance before, and it can happen if the first token in the path is the same as the last in the path. It will be lower because some fees will be taken.
If the rebalance manager provides the same input token and output token, there will be undesired behavior.
consider the following scenario: the basket has a single asset: dai, with 1M dai. the rebalance manager provides the path [dai,weth,dai]. the target token is dai, so oldBalance is 1M dai (that's the balance before the swap). after the swap the balance of dai will be less than 1M because of fees, so input will be equal to <1M - 1M =underflow. therefore input will be a huge amount. therefore the require statement will pass:
require(trade.minimumReturn <= input, "INSUFFICIENT_OUTPUT_AMOUNT");
this will make trade.minimumReturn pointless and will allow sandwich attack of any amount.
If there are multiple swaps in the rebalance, it can even cause more damage, because input is passed to the next swap, so it will swap from the basket more than it should.
manual review
use safe math:
input = IERC20(targetToken).balanceOf(address(basket)).sub(oldBalance);
#0 - 0xleastwood
2022-01-23T04:24:27Z
Duplicate of #43
certora
use of the function transfer to transfer ether is not recommended and may cause problems. Here is an article that explains why: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
use call instead of transfer: msg.sender.call.value(intermediateTokenBalance)("");
#0 - loki-sama
2022-01-03T10:03:33Z
duplicate #175
certora
https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/factories/PieFactoryContract.sol#L48 defaultCut can have duplicates. It will cause denial of service for anyone who tries the add a basket, because diamond doesn't allow duplicates: https://github.com/pie-dao/initialisable-diamond/blob/master/contracts/libraries/LibDiamond.sol#L126 https://github.com/pie-dao/initialisable-diamond/blob/master/contracts/libraries/LibDiamond.sol#L150
If the owner accidently adds the same facet twice no baskets can be added until they remove the duplicates.
check duplicates in addFacet, and if there are, revert.
#0 - 0xleastwood
2022-01-23T05:17:29Z
Duplicate of #189
certora
Lack of precision, the division should be last.
move the div(10**18) to the end of the expression
#0 - loki-sama
2022-01-07T12:52:40Z
duplicate #155
certora
already submitted, but wanted to change, so ignore my previous submission.
receive function is dangerous because it allows anyone to send ether, but it should only be called by the intermediate token.
this might confuse users to send ether using this function before calling joinTokenEth, and then their ether will be locked forever.
add:
require(msg.sender == address(INTERMEDIATE_TOKEN));
in recieve
#0 - 0xleastwood
2022-01-23T04:37:06Z
Duplicate of #253
🌟 Selected for report: certora
certora
https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/EthSingleTokenJoin.sol#L15
msg.value might be different than _joinTokenStruct.inputAmount
add:
require(msg.value == _joinTokenStruct.inputAmount);
#0 - 0xleastwood
2022-01-23T04:54:41Z
I think this is valid, the function will likely revert unknowingly to the user who calls this function.
450.6605 USDC - $450.66
certora
https://github.com/code-423n4/2021-12-amun/blob/main/contracts/bridge/contracts/amunBasketBridge/PolygonERC20Wrapper.sol#L39 the comment says that this function should mint tokens to the users, but it doesn't.
#0 - loki-sama
2022-01-03T11:47:55Z
Comment could be changed
#1 - 0xleastwood
2022-01-23T04:55:20Z
Duplicate of #275
🌟 Selected for report: certora
1001.4677 USDC - $1,001.47
certora
division is rounded down so users pay less than they should.
https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L163
It's better to use mulDivRoundingUp when calculation user's debt, so the rounding error will be in favor the system.
example from uniswap:
https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3Pool.sol#L800
use mulDivRoundingUp from FullMath.sol:
https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/FullMath.sol
450.6605 USDC - $450.66
certora
The minimum amount in BasketFacet should be low, however, some tokens have low precision and it is a high amount.
for example, wbtc has 8 decimals, so the minimum amount would be 470$. and for tether gold, the minimum amount would be 1800$.
change MIN_AMOUNT to 10**4