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: 4/12
Findings: 7
Award: $3,446.19
🌟 Selected for report: 13
🚀 Solo Findings: 2
0xRajeev
addToken does not check for token being added a duplicate of what was already added. If a duplicate token is added, removeToken only removes the first matching token and the later duplicates still remain. With the vault[token] deleted, this may lead to undefined behavior.
Manual Analysis
Check for duplicate token being added and do not add such duplicates.
#0 - Haz077
2021-09-21T12:36:04Z
require(vaults[_token] == address(0), "!_token") already checks for duplicated tokens.
#1 - uN2RVw5q
2021-09-29T17:41:37Z
I think this is a valid issue. Effectively a duplicate of https://github.com/code-423n4/2021-09-yaxis-findings/issues/3
The check require(vaults[_token] == address(0), "!_token") did not exist when the contracts were part of the contest.
I would also change the severity to high risk, just like in the duplicated issue.
#2 - Haz077
2021-09-29T18:23:52Z
My bad, I missed that the check was added after the contest ended.
#3 - GalloDaSballo
2021-10-13T23:22:39Z
Duplicate of #3
0xRajeev
There is an assumption in LegacyController.vault() that the vault will have enough tokens[0] to cover the balance difference. If not, the user may receive less than amount requested and balance funds get lost/locked unless the vault withdraws from the strategy. This is not clear.
Manual Analysis
Ensure that vault will have enough tokens[0] in it or withdraws from strategy to cover the withdrawal request.
#0 - GainsGoblin
2021-09-30T16:52:52Z
Duplicate of #28
#1 - GalloDaSballo
2021-10-14T17:07:03Z
Duplicate of #28
37.6774 YAXIS - $146.94
0xRajeev
User may receive less than the eligible amount per the shares being withdrawn. It is not clear under what conditions this happens but needs to be documented and user warned.
Manual Analysis
Evaluate under what conditions this is true and ensure this is documented and user warned.
#0 - Haz077
2021-09-21T11:59:10Z
I don't see where is the issue.
#1 - uN2RVw5q
2021-10-01T17:25:16Z
AFAIU, it could be possible that if the contract doesn't have enough balance for IERC20(_output), the user may actually be burning more shares than required to get the same amount of tokens.
There are two ways to proceed:
withdraw to be more like withdrawp(uint256 _shares, address _output, uint256 _minAmount). After the _amount computation, there can be a check require(_amount >= _minAmount) to guarantee that at least _minAmount tokens are returned. This helps avoid users accidentally burning more tokens than needed.So I'm not sure about the "sponsor disputed" tag for this issue. I think this should be reconsidered.
#2 - uN2RVw5q
2021-10-02T16:11:07Z
#3 - GalloDaSballo
2021-10-13T23:20:50Z
Duplicate of #121
🌟 Selected for report: 0xRajeev
139.546 YAXIS - $544.23
0xRajeev
The manager.allowedVaults check is missing for add/remove strategy like how it is used in reorderStrategies(). This will allow a strategist to accidentally/maliciously add/remove strategies on unauthorized vaults.
Given the critical access control that is missing on vaults here, this is classified as medium severity.
Manual Analysis
Add manager.allowedVaults check in addStrategy() and removeStrategy()
#0 - uN2RVw5q
2021-10-03T19:15:19Z
Implemented in https://github.com/code-423n4/2021-09-yaxis/pull/36
#1 - GalloDaSballo
2021-10-14T15:40:49Z
Sponsor has acknowledged and mitigated by adding further access control checks
🌟 Selected for report: 0xRajeev
139.546 YAXIS - $544.23
0xRajeev
A malicious strategist can halt the entire protocol and force a permanent shutdown once they observe that governance is trying to set a new strategist and they do not agree with that decision. They may use the 7 day window to halt the protocol. The access control on setHalted() should be onlyGovernance.
Manual Analysis
Change access control to onlyGovernance from onlyStrategist for setHalted()
#0 - uN2RVw5q
2021-10-03T15:58:06Z
Implemented in https://github.com/code-423n4/2021-09-yaxis/pull/35
#1 - GalloDaSballo
2021-10-14T15:42:44Z
Agree that such critical functionality should be limited to the highest permission access role. Sponsor has mitigated
20.9319 YAXIS - $81.63
0xRajeev
Deflationary tokens deduct a fee in transfers which causes their amount transferred to be less than that specified. Given the intended support of stablecoins: DAI, USDC and USDT, USDT has the potential to become deflationary because of non-0 fee functionality that maybe enabled. The codebase doesn't seem to support accounting for deflationary tokens because it does not calculate the before-and-after balances to account for such token transfers.
Manual Analysis
Evaluate support for tokens that have the potential to become deflationary such as USDT.
#0 - Haz077
2021-09-22T16:58:49Z
It's same as #13 in my opinion.
20.9319 YAXIS - $81.63
0xRajeev
Zero-address checks during initialization of address type variables is a best-practice. This is missing in several places of the protocol contracts.
Manual Analysis
Add zero-address checks
#0 - GalloDaSballo
2021-10-13T22:48:07Z
Agree with finding Sponsor mitigated I appreciate that the Warden linked all instances of the finding making it easy to mitigate
🌟 Selected for report: 0xRajeev
46.5153 YAXIS - $181.41
0xRajeev
Critical contract parameters of non-address types should have sanity/threshold checks in constructor/initialize/setter to ensure they are relevant from the application-logic’s perspective. Accidentally setting arbitrary values may lead to reverts or financial loss by transferring/withholding more rewards/fees than specified.
While most setter functions have such checks, setTotalDepositCap() is missing such a check which may accidentally allow a strategist to set a very low value e.g. 0 which will prevent deposits and cause DoS.
Manual Analysis
Add sanity/threshold check setTotalDepositCap() to make sure totalDepositCap is above a minimum threshold.
#0 - GalloDaSballo
2021-10-13T23:12:55Z
Agree that setTotalDepositCap should do a sanity check
🌟 Selected for report: 0xRajeev
46.5153 YAXIS - $181.41
0xRajeev
The use of onlyEnabledConverter modifier (which checks that converter is non-zero) on legacyDeposit() implies an implicit assumption that converter may be 0 address. However, this modifier is not used in withdraw() function where converter plays a critical role.
Manual Analysis
Evaluate if the modifier is required on withdraw() function.
#0 - GalloDaSballo
2021-10-13T23:32:35Z
Sponsor acknowledged
🌟 Selected for report: 0xRajeev
46.5153 YAXIS - $181.41
0xRajeev
No use of notHalted modifier from manager.halted() in any LegacyController functions. It is not clear if this is the specification. If not, such functions (including legacyDeposit) may continue to interact with the protocol even after the Manager halts the protocol.
Manual Analysis
If this is according to the specification, highlight it. If not, add notHalted functionality to LegacyController functions.
#0 - GalloDaSballo
2021-10-13T23:30:36Z
Sponsor acknolwedged, am fine with finding
🌟 Selected for report: 0xRajeev
46.5153 YAXIS - $181.41
0xRajeev
Maximum approvals are widely considered as unsafe if the approved contract becomes compromised/malicious.
Manual Analysis
Where possible, consider approving as needed for the amounts required as done in VaultHelper.
#0 - GalloDaSballo
2021-10-13T23:35:39Z
Agree with finding, sponsor mitigated
🌟 Selected for report: 0xRajeev
15.052 YAXIS - $58.70
0xRajeev
The deposit and withdraw functions in VaultHelper can prevent expensive logic and external calls to token functions and Vault functions from executing for zero amount invocations by checking for zero amount deposits/withdrawals here instead of within Vault functions. External calls themselves cost 2600 gas since the Berlin upgrade.
Manual Analysis
Check for zero amounts and return instead of executing all the logic including external calls.
#0 - uN2RVw5q
2021-10-03T15:20:36Z
I would say that this should just be a frontend check. By adding a zero check, you are also making a regular transaction more expensive (although by only about 30 gas or so) The case where amount is 0 should be rather rare, adding the overhead only to save gas in that case is not worth it.
#1 - GalloDaSballo
2021-10-12T21:30:42Z
Agree with finding, and find front-end mitigation from sponsor sufficient
6.7734 YAXIS - $26.42
0xRajeev
Missing check on lengths of two arrays (tokens & amounts) being the same depositMultipleVault(). Instead of postponing this check to vault.depositMultiple(), adding it here can save gas when mismatch.
There are 3 external calls (safeTransfer + 2 safeApproves) performed for each token. Each such call costs 2600 gas besides the external function logic. I the event of a mismatch all that gas consumed is lost.
Manual Analysis
Move this check require(_tokens.length == _amounts.length, “!length"); to beginning of depositMultipleVault().
#0 - Haz077
2021-09-20T19:26:48Z
Same require that is mentioned in #20
#1 - GalloDaSballo
2021-10-12T21:48:43Z
Agree with findings Duplicate of #20
#2 - GalloDaSballo
2021-10-12T21:49:23Z
Duplicate of #20
6.7734 YAXIS - $26.42
0xRajeev
The removal of the last token in the array can be optimized by checking if index == k-1, i.e. last element match, when there is no need to copy and simply popping will do. This will avoid the copy which involves several SLOADs/SSTOREs.
Manual Analysis
Example: Check if index != k-1 for tokens[_vault][index] = tokens[_vault][k-1];
#0 - GalloDaSballo
2021-10-12T22:07:54Z
Agree with sponsor and severity, wouldn't mind not mitigating as it's very minor
🌟 Selected for report: 0xRajeev
15.052 YAXIS - $58.70
0xRajeev
Storage slots are allocated based on the declaration order of state variables in contract definitions. For types less than 256 bits, they can be packed by the compiler if more than one fit into the same 32B storage slot. This reduces the number of storage slots but may increase runtime gas consumption because of masking the other shared variables in slot. However, if variables used together in function logic are packed in the same slot, it allows the compiler to optimize SLOADs/SSTOREs.
Example: An example of this is the declaration of the halted boolean state variable. Given the current declaration order, this occupies a full slot because booleans are internally represented by uint8 and the neighbouring declarations are uint256 which need a full slot for themselves.
Moving the halted bool next to governance address variable declaration will allow those two to share a slot. This reduces one slot and also should not incur extra masking gas overhead at runtime because governor and halted are used in onlyGovernance and notHalted modifiers respectively which are typically used together.
Manual Analysis
Move halted declaration to immediately after governance declaration. Also, consider the declaration order of all state variables across contracts for such packing possibilities.
#0 - uN2RVw5q
2021-10-03T15:40:57Z
This is technically a valid gas optimization. Even ignoring the compiler optimization part, packing governance and notHalted boolean member in the same slot would mean that if a single function has the modifiers onlyHalted and onlyGovernance, the second sload would only cost 100 gas. In the best case, the compiler may remove the second sload and instead reuse the previous sload(...) from stack.
I would recommend changing the tag to "sponsor confirmed".
This is a trivial change; will submit a PR for this.
#1 - uN2RVw5q
2021-10-03T15:46:01Z
Implemented in https://github.com/code-423n4/2021-09-yaxis/pull/33.
#2 - GalloDaSballo
2021-10-12T22:06:32Z
Sponsor may have disputed at the time, however they ended up mitigating the issue Packing by changing order does save gas, valid finding
0xRajeev
There are few places across contracts where the same state variables are read multiple times or the same external calls are made multiple times within a function. Caching state variables or results from external calls in local/memory variables avoids SLOADs and CALLs to save gas. Warm SLOADs cost 100 gas and CALLs cost 2600 gas after Berlin upgrade. MLOADs cost only 3 gas units.
Manual Analysis
Cache state variables or results from external calls in local/memory variables to save gas.
#0 - Haz077
2021-09-21T17:13:20Z
I don't see where does the code mentioned makes same external calls more than once except for tokens[i].balanceOf(address(this)) in convert() and it has to be twice because output changes after removing liquidity.
#1 - uN2RVw5q
2021-10-03T19:22:33Z
This is implemented in https://github.com/code-423n4/2021-09-yaxis/pull/31. And is a duplicate of https://github.com/code-423n4/2021-09-yaxis-findings/issues/143
#2 - uN2RVw5q
2021-10-03T19:38:01Z
I think this is a valid finding. The tag "sponsor disputed" should be removed.
The first two suggestions are implemented in https://github.com/code-423n4/2021-09-yaxis-findings/issues/56
The last one is a duplicate.
#3 - GalloDaSballo
2021-10-12T22:14:02Z
Duplicate of #143
🌟 Selected for report: 0xRajeev
15.052 YAXIS - $58.70
0xRajeev
withdrawalProtectionFe() appears to be the same for all tokens and therefore the parameter _token and the onlyToken() modifier are unused. Removing it and the modifier can save gas.
Manual Analysis
Remove parameter and modifier to save gas.
#0 - uN2RVw5q
2021-10-03T19:43:07Z
https://github.com/code-423n4/2021-09-yaxis/pull/28#issuecomment-931650139 mentions that removing the parameter _token would break the interface.
The check onlyToken(_token) may still be removed, though. But I'll leave the decision to the other devs.
#1 - Haz077
2021-10-08T17:51:15Z
I would say if that would break the interface then this issue should be skipped even if it's still valid in my opinion, we can leave the acknowledged label.
#2 - GalloDaSballo
2021-10-12T22:17:08Z
Sponsor acknowledged, agree with finding
4.064 YAXIS - $15.85
0xRajeev
The solc version used is 0.6.12 which was released in July 2020 (one year is a long time for Solidity given the fast release pace) and is two breaking releases behind. This misses several optimizations and the built-in arithmetic checks in 0.8.x.
Manual Analysis
Consider upgrading to 0.7.x if not 0.8.x.
#0 - GalloDaSballo
2021-10-12T22:57:34Z
Duplicate of #98
#1 - loudoguno
2021-10-19T13:29:32Z
adding duplicate label as per judge in findings sheet
20.9319 YAXIS - $81.63
0xRajeev
OpenZeppelin’s safeApprove reverts for non-0 to non-0 approvals. This is considered in a few places where safeApprove is performed twice with the first one for 0 and then for the desired allowance. However, there are uses of safeApprove where it is called only once for the desired allowance but it is not clear that the current allowance is guaranteed to be zero. In such cases, safeApprove may revert.
Manual Analysis
Approve to 0 first while using safeApprove or use increaseAllowance instead.
#0 - uN2RVw5q
2021-10-02T16:18:26Z
Implemented and merged: https://github.com/code-423n4/2021-09-yaxis/pull/13
#1 - GalloDaSballo
2021-10-13T23:34:59Z
Agree with finding, sponsor mitigated