Platform: Code4rena
Start Date: 09/09/2021
Pot Size: $60,000 USDC
Total HM: 24
Participants: 12
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 14
Id: 30
League: ETH
Rank: 10/12
Findings: 2
Award: $502.81
🌟 Selected for report: 5
🚀 Solo Findings: 0
🌟 Selected for report: pauliax
46.5153 YAXIS - $181.41
pauliax
A theoretical issue is that the decimals of USDC may change as they use an upgradeable contract so you cannot assume that it stays 6 decimals forever: balances[1] = stableSwap3Pool.balances(1).mul(10**12); // USDC
A simple solution would be to call .decimals() on token contract to query it on the go. Then you will not need to hardcode it but gas usage will increase.
#0 - Haz077
2021-09-25T16:04:11Z
I believe that there were other submissions mentioning issues with the decimal (for example: handling tokens with more than 18 decimal points).
But I think calling .decimals() from the tokens contract will fix those issues, even though it will cost more gas, so I think this issue should be accepted.
#1 - uN2RVw5q
2021-09-27T16:48:48Z
I would say that it's extremely unlikely that USDC will upgrade the contract and change its decimals, as this would break many existing contracts. Perhaps this should simply be documented. Even if it happens, it's very likely that this would be made public in advance, allowing the tokens to be withdrawn.
IMO, changing 10**12 to 10**18/USDC.decimals() is an unnecessary waste of gas (more than 5000 additional gas, even just counting the call, and storage read)
#2 - GalloDaSballo
2021-10-13T22:52:05Z
I believe the finding to be valid, but wouldn't expect sponsor to have to mitigate
20.9319 YAXIS - $81.63
pauliax
"totalDepositCap is the maximum amount of value that can be deposited", however, it is compared against totalSupply() which is the number of shares and may be different than the deposited amount.
Either track the amounts and check against the sum of them or remove the confusion by naming this variable something like totalSharesCap and updating comments.
#0 - Haz077
2021-09-25T16:55:36Z
totalSupply() can't be different than deposited amount as it increase/decrease with deposit/withdraw.
#1 - GalloDaSballo
2021-10-14T00:30:35Z
Duplicate of #25
pauliax
There are places where the same storage variable is accessed multiple times in the same function. It would be more gas efficient to cache these variables and re-use them where necessary. E.g. function addStrategy accesses _vaultDetails[_vault] 5 times. Similarly with function withdrawAll and other functions.
Cache storage access and reuse variables.
#0 - GalloDaSballo
2021-10-12T23:06:46Z
Duplicate of #56
Would like to see links to examples and mitigations, instead of quick copy paste of function names
2.7432 YAXIS - $10.70
pauliax
This line is useless addition as _shares hasn't been assigned a value before so it starts with 0: _shares = _shares.add(_amount); and is essentially identical to: _shares = _amount;
In this case you do not even need this separate value and can use _amount instead.
#0 - GalloDaSballo
2021-10-12T23:08:35Z
Duplicate of #118
2.7432 YAXIS - $10.70
pauliax
notHalted modifier is not needed in function depositMultiple as it will be checked in function deposit anyway.
Remove notHalted from function depositMultiple.
#0 - Haz077
2021-09-27T17:56:02Z
Same as #70
#1 - GalloDaSballo
2021-10-12T23:10:04Z
Duplicate of #70
6.7734 YAXIS - $26.42
pauliax
There are several loops that use uint8 for an index parameter (i). It does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint8 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.
You can try and see yourself: // SPDX-License-Identifier: MIT pragma solidity 0.6.12; contract VaultHelper { function loopUint8( uint256[] calldata _amounts ) external { uint sum = 0; for (uint8 i = 0; i < _amounts.length; i++) { // [1,2,3,4,5,6] 23892 require(_amounts[i] > 0); sum += _amounts[i]; } } function loopUint256( uint256[] calldata _amounts ) external { uint sum = 0; for (uint256 i = 0; i < _amounts.length; i++) { // [1,2,3,4,5,6] 23768 require(_amounts[i] > 0); sum += _amounts[i]; } } }
Remix IDE
Replace uint8 with uint256 in loop iterations.
#0 - GalloDaSballo
2021-10-12T23:01:27Z
Agree with finding, just KIS and use uint256
🌟 Selected for report: pauliax
15.052 YAXIS - $58.70
pauliax
rate state variable in MinterWrapper is practically useless as it is not used in any meaningful way. event InsuranceClaimed is declared but never emitted. Same with events VaultManagerSet and ControllerSet.
Consider removing dead code.
#0 - GalloDaSballo
2021-10-12T23:05:37Z
Evidence from code provided does agree with warden finding
Sponsor disputed
Will keep issue as valid as it may be because of some issue with the repo, however sponsor shouldn't have to mitigate as at worst this is a waste of gas
🌟 Selected for report: pauliax
15.052 YAXIS - $58.70
pauliax
function _checkToken can be moved to modifier checkToken as it is a private function that is only used by this modifier. This will reduce the number of extra calls and thus reduce the gas.
Consider moving this function inside the modifier to reduce gas usage.
#0 - Haz077
2021-09-25T17:42:46Z
I'm not sure if that will optimize gas that much, also I think code is more organized that way. So in my opinion this issue should be disputed.
#1 - uN2RVw5q
2021-09-27T16:31:41Z
I would say that this is a valid issue. Moving the function body into the modifier would save an internal function call: around 20-30 gas. Since checkToken is frequently used throughout the codebase, I would also recommend changing the gas.
This is implemented in https://github.com/code-423n4/2021-09-yaxis/pull/20
#2 - GalloDaSballo
2021-10-12T23:09:25Z
Sponsor acknowledged and mitigated
🌟 Selected for report: pauliax
15.052 YAXIS - $58.70
pauliax
functions depositVault, depositMultipleVault and withdrawVault in VaultHelper could require _amount > 0 to prevent useless transfers.
Add require _amount > 0 statements to mentioned functions.
#0 - Haz077
2021-09-25T17:00:47Z
I think this should be labelled gas optimization.
#1 - uN2RVw5q
2021-10-01T15:36:53Z
I think this should not be done in the smart contract, but rather as a front end check. Say, add this check in JavaScript before making the call. If we really want to, then this can be added as a comment in the documentation, for anyone integrating with the protocol.
Doing this in the smart contracts effectively means adding a if (amount > 0) { ...} check and skip the transfers for non-zero amounts. My rough calculation is that this check adds around 25 gas for every call to the function. The case of amount = 0 is an exceptional case, so adding this 25 gas overhead just to save gas in this rare case should be avoided.
I would recommend changing the severity to documentation.
#2 - GalloDaSballo
2021-10-13T22:25:40Z
Agree with findings in theory and with sponsor mitigation, FE check is completely fine