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: 6/30
Findings: 5
Award: $3,426.19
🌟 Selected for report: 5
🚀 Solo Findings: 0
kenzo
When joining a basket, the function verifies that the total supply + tokens the user asks to mint is smaller than the basket's max supply. However, this doesn't take into account the fact that additional tokens will be minted if there's an entry fee beneficiary share.
More tokens will be minted than basket's max cap.
Upon user entering the pool, joinPool will check that the requested amount does not surpass the basket's max cap: (Code ref)
require( totalSupply.add(_amount) <= this.getCap(), "MAX_POOL_CAP_REACHED");
It will mint _amount to the user: (Code ref)
LibERC20.mint(msg.sender, _amount);
And if there's a beneficiary share of the entry fee, it will mint additional tokens to the beneficiary: (Code ref)
LibERC20.mint(bs.feeBeneficiary, feeBeneficiaryShare);
These tokens were not checked against the token's maxCap and therefore it can go over the limit.
Add the feeBeneficiaryShare to the check against maxCap.
#0 - loki-sama
2021-12-29T11:43:55Z
Duplicate #283
kenzo
SingleNativeTokenExitV2 takes as input from the user a deadline for the trades. However, it does not use this input for the actual trade but sets the deadline to be block.timestamp.
Trades will not work as expected. User might set a deadline for the trade but his trade will get executed regardless.
This issue is present in a few contracts in the repo - SingleNativeTokenExitV2, SingleTokenJoinV2. If I submit both in one issue, but the judge will decide to reward them separately, he would not be able to do so for me as I submitted them all in one issue. So this is why I am submitting them in separate issues. If it is will be rewarded only as one issue, the judge will close the duplicate. Thank you.
We can see the input struct has deadline as a parameter, however, _exit passes as deadline block.timestamp, and not the input deadline.
Send to the router _exitTokenStruct.deadline instead of block.timestamp. (Code ref)
#0 - loki-sama
2021-12-29T12:44:36Z
duplicate #112
#1 - 0xleastwood
2022-01-22T03:58:43Z
Duplicate of #47
811.1888 USDC - $811.19
kenzo
SingleNativeTokenExitV2 allows the user to exit and execute trades via multiple exchanges. When finishing the trades and sending a single output token back to the user, the contract takes that token from the last swap in the first exchange's trades. There is nothing in the struct that signifies this will be the output token, and this also impairs the exit functionality.
Let's say a basket only holds token TOKE, and user would like to exit to DAI. But there's no exchange with good liquidity for TOKE -> DAI. So the user crafts a trade to exchange TOKE for WOKE in exchange A, and then exchange WOKE for DAI in exchange B, to finally receive back DAI. The contract will not let him do so, as the output token is taken to be the output token of the first exchange - WOKE in our example.
In exit, the output token is taken to be the last token exchanged in the first exchange: (Code ref)
address[] calldata path = _exitTokenStruct .trades[0] .swaps[_exitTokenStruct.trades[0].swaps.length - 1] .path; IERC20 outputToken = IERC20(path[path.length - 1]); //this could be not the target token
This manifests the issue I detailed above.
Have the outputToken be a parameter supplied in ExitTokenStructV2.
kenzo
In various singleJoinExit files, native token is being sent to the user using transfer function.
Although in the past it was it was recommended, these days the general recommended way to send native token is using call.
This is because of 2 reasons -
transfer does not fully protect from reentrancy as gas costs can change. (In your scenario there is no risk of reentrancy.)transfer will only forward small amount of gas. So if for example the msg.sender is a smart contract that wants to do something with received ether (eg. deposit in some other platform), he will not be able to do it. This kinda breaks the composability of the ecosystem.
You can read a little more about it in this Consensys article.The following files use transfer: EthSingleTokenJoin, EthSingleTokenJoinV2, SingleNativeTokenExit, SingleNativeTokenExitV2. All of them use it in the last line of external function so there's no reentrancy concern.
Consider if using call will be better for your use case. I think that there's no drawback to moving to call, and you'll gain composability.
#0 - loki-sama
2022-01-03T12:23:02Z
duplicate #175
59.8749 USDC - $59.87
kenzo
An early division in calcOutStandingAnnualizedFee will lose accuracy of the fees calculation.
Lost fees for the protocol.
calcOutStandingAnnualizedFee uses the following formula to calculate the fees: (Code ref)
return totalSupply.mul(annualizedFee).div(10**18).mul(timePassed).div( 365 days );
Since it is dividing by 10**18 in the middle of the calculation, the remainder of that division will be lost. This is why in Solidity, division should come in the end.
Move div(10**18) to the end of the calculation.
By the way, you might wanna consider changing your fee structure to a smaller basis. Making the base 1e18 gives you granularity that you might not need. For example, if the annualized fee is planned to be 1%, instead of it being 1e15 out of 1e18 it can be 100 out of 10000. This will help prevent overflow if that was the reason you chose to divide in the midst of the calculation.
#0 - 0xleastwood
2022-01-23T03:07:44Z
I think this qualifies as low. Typically value leak from the protocol would be considered medium, however, this value isn't actually lost but just slightly incorrect.
🌟 Selected for report: kenzo
1001.4677 USDC - $1,001.47
kenzo
A basket is not usable if totalSupply = 0. When initializing a basket, the initial supply is passed as parameter, but the contract does not verify that that parameter is bigger than 0.
A basket may be unusable after deployment.
A basket should never have totalSupply = 0, as joinPool divides by totalSupply: (Code ref)
uint256 tokenAmount = balance(address(token)).mul(_amount.add(feeAmount)).div(totalSupply);
When the basket's ERC20 facet is initialized, it does not check whether the initialSupply parameter is bigger than 0. The mint function will also not revert.
So, the basket can be deployed with initialSupply = 0 and be unusable.
Note: it is not a part of the audit, but PieFactoryContract also does not check initialSupply.
Add a check that requires initialSupply > 0.
#0 - 0xleastwood
2022-01-23T03:26:04Z
I consider this a useful check, but it should be made low.
kenzo
When running a loop until the length of a storage array is reached, the length of the array can be cached in memory instead of reading it every iteration from storage.
Around 97 gas can be saved per iteration.
For example when iterating over basket tokens in joinPool (Code ref):
for (uint256 i; i < bs.tokens.length; i++) {
bs.tokens.length can be stored in a local uint.
Cache storage arrays length in loops.
#0 - 0xleastwood
2022-01-23T22:13:44Z
Duplicate of #249
14.2759 USDC - $14.28
kenzo
As the title says. The constant keccak variables are defined all throughout the diamond storage contracts. So they are used often.
Warden TomFrench has written a good description of the issue here, so I'm copying it: (Cheers Tom... 🙂🥂)
" In a number of places a keccak("string") expression is assigned to a constant variable. Due to how constant variables are implemented this results in the hash being recomputed each time that the variable is used, spending the gas necessary to perform this action.
If these variables were to be immutable this hash is calculated once at deploy time and then the result is saved to be used directly at runtime rather than recalculating, saving the cost of hashing.
Change all constant hashes to be immutable "
#0 - 0xleastwood
2022-01-23T22:11:40Z
Duplicate of #281
35.2492 USDC - $35.25
kenzo
uniSwapLikeRouter in SingleNativeTokenExitV2 is initialized but unused, probably a remainder of copying SingleNativeTokenExit. You can remove it to save deployment gas cost and for clarity.
#0 - loki-sama
2022-01-11T12:58:15Z
duplicate #286
🌟 Selected for report: kenzo
kenzo
Upon withdraw and withdrawTo, PolygonERC20Wrapper calls _mint and _burn, presumably to be able to generate a burn event.
But it can just emit a burn event, just like how it emits a deposit event in deposit.
This will save the gas of calling and executing the ERC20 functions.
Code ref
Change _mint and _burn to emit Transfer(user/recipient, address(0), amount).
🌟 Selected for report: kenzo
kenzo
Since the minting and burning is all virtual, PolygonERC20Wrapper does not need to implement the full ERC20, but can just implement the needed functions for the bridge functionality and emit a burn event (transfer to 0 address) when needed.
Save gas and complexity.
The only issue might be if Polygon for some reason declines to map it, but I see no reason for them to do so.
kenzo
EthSingleTokenJoin contains a general empty receive method to receive ether.
It is probably there to receive unwrapped funds from INTERMEDIATE_TOKEN, however, it does not check the funds' source, so everybody can send native token to the contract.
Users might accidently send ether to this contract. These funds will be lost (they will get stuck as a native token which can not get converted to INTERMEDIATE_TOKEN, which is what the rest of the functions work upon.)
This issue is present in a few contracts in the repo - EthSingleTokenJoin, EthSingleTokenJoinV2, SingleNativeTokenExit, SingleNativeTokenExitV2. If I submit all these files in one issue, but the judge will decide to reward them separately, he would not be able to do so for me as I submitted them all in one issue. So this is why I am submitting them in separate issues. If it is will be rewarded only as one issue, the judge will close the duplicates. Thank you.
This is the general receive function, which does not check funds source: (Code ref)
receive() external payable {}
As the funds should only come upon unwrapping from INTERMEDIATE_TOKEN, check in the receive function that the msg.sender is INTERMEDIATE_TOKEN.
#0 - 0xleastwood
2022-01-23T04:30:35Z
Duplicate of #253