Platform: Code4rena
Start Date: 12/07/2022
Pot Size: $35,000 USDC
Total HM: 13
Participants: 78
Period: 3 days
Judge: 0xean
Total Solo HM: 6
Id: 135
League: ETH
Rank: 9/78
Findings: 4
Award: $2,104.07
🌟 Selected for report: 2
🚀 Solo Findings: 0
🌟 Selected for report: hansfriese
Also found by: panprog
1005.6038 USDC - $1,005.60
https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/VaultTracker/VaultTracker.sol#L124 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/VaultTracker/VaultTracker.sol#L132
With most functions in VaultTracker.sol, users can call them only once after maturity has been reached.
So from the second call of any functions after maturity, it will revert and users might lose their funds or interests.
The main problem is that vlt.exchangeRate might be larger than maturityRate after maturity here.
Then it will revert from the second call with uint underflow here.
So such scenario is possible.
Solidity Visual Developer of VSCode
We should save vlt.exchangeRate = maturityRate when maturityRate > 0 and exchangeRate > maturityRate.
vlt.exchangeRate = (maturityRate > 0 && maturityRate < exchangeRate) ? maturityRate : exchangeRate;
There are several places to modify for safety.
#0 - JTraversa
2022-07-15T22:17:40Z
I dont actually disagree with most of the warden's claims, just note that all of this would need to be intentional on the part of the user, and could not happen under any normal workflow of the protocol.
E.g.:
Alice splits 1000 USDC into zcToken and nToken using splitUnderlying() After the market is martured, she withdraws back 1000 USDC using combineTokens()
This interaction relies on a user calling combineTokens past maturity, which is 1. not possible from our UI 2. not beneficial in any way whatsoever, as redemption is explicitly done post maturity through redeemZcToken, + not using redeemZcToken would also fail to accrue post maturity interest.
The same can be said of:
Also, if she claims interest first before calling combineTokens(), she can't get paid back here 1000 USDC forever because removeNotional() will revert here for the same reason.
There is no use case to calling combineTokens after maturity, meaning this specifically isn't an issue in that direction either.
Just disagreeing with severity because of the lack of potential impact in any reasonable operation of the protocol. A user would have to do this through a script, and the workflow would be unlikely to happen on accident.
#1 - robrobbins
2022-08-03T22:48:48Z
#2 - 0xean
2022-08-24T22:01:24Z
I am somewhere between leaving this as high and downgrading to medium. I understand the sponsors points, but think its a pretty nasty rough edge to leave out there if a user interacted with the contract through etherscan or some other UI with less guard rails.
I am going to downgrade to medium based on it being somewhat self inflicted and the probability of it therefore very low.
🌟 Selected for report: hansfriese
Also found by: jonatascm
Swivel.setFee() is implemented wrongly. Swivel.feenominators won't be set as expected.
This function has a parameter "i" for the index of the new fee denomination but it isn't used during the update.
Solidity Visual Developer of VSCode
This line should be modified like below.
feenominators[i[x]] = d[x];
#0 - JTraversa
2022-07-20T07:45:29Z
Given this allows us to change fees post initialization, and doesnt lead to the leakage of value or loss of funds, but a potential issue for admins solely (in a rare edge case where fees would even need to be changed), I might consider this low risk?
#1 - bghughes
2022-08-03T01:18:39Z
Given this allows us to change fees post initialization, and doesnt lead to the leakage of value or loss of funds, but a potential issue for admins solely (in a rare edge case where fees would even need to be changed), I might consider this low risk?
I agree with the warden here given that this is an incorrect implementation of the intended functionality. The warden's suggestion shows that the function was not working as intended; if in production, for instance, this was not caught then calls to setFee would not work as intended and not set any fees across the markets. Instead, it would only populate the zero index of the 2nd-demensional array uint16[4] public feenominators; losing these values [zcTokenInitiate, zcTokenExit, vaultInitiate, vaultExit] and breaking functionality.
#2 - JTraversa
2022-08-05T00:26:43Z
Yeah I was kind of on the fence on this one thinking the method wouldnt impact the current feenominators in the way youve stated, but because it actually could have an impact, and the event itself also would be misleading, removing the disagreement.
#3 - robrobbins
2022-08-08T19:31:27Z
🌟 Selected for report: 0xDjango
Also found by: 0x1f8b, 8olidity, Bahurum, Lambda, arcoun, caventa, csanuragjain, hansfriese, joestakey, jonatascm, oyc_109, ronnyx2017
48.5491 USDC - $48.55
https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Creator/ZcToken.sol#L112-L114 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Creator/ZcToken.sol#L133
ZcToken.withdraw() and ZcToken.redeem() will always revert when msg.sender != holder.
These 2 functions will work only when users withdraw/redeem from their balances.
When we check allowance here, it reverts when allowance is greater than required amount.
if (allowed >= previewAmount) { revert Approvals(allowed, previewAmount); }
Solidity Visual Developer of VSCode
We should fix like below with 2 parts, here and here.
if (allowed < previewAmount) { revert Approvals(allowed, previewAmount); }
#0 - JTraversa
2022-07-20T07:26:49Z
Duplicate of #129
#1 - robrobbins
2022-08-02T22:45:10Z
Duplicate of #180. Resolved there
#2 - bghughes
2022-08-03T14:45:35Z
Duplicate of #129
🌟 Selected for report: joestakey
Also found by: 0x1f8b, 0x52, 0xDjango, 0xNazgul, 0xNineDec, 8olidity, Avci, Bahurum, Bnke0x0, Chom, ElKu, Funen, GimelSec, JC, Junnon, Kaiziron, Meera, PaludoX0, Picodes, ReyAdmirado, Sm4rty, Soosh, Waze, _Adam, __141345__, ak1, aysha, benbaessler, bin2chen, c3phas, cccz, cryptphi, csanuragjain, defsec, exd0tpy, fatherOfBlocks, gogo, hake, hansfriese, itsmeSTYJ, jonatascm, kyteg, mektigboy, oyc_109, pashov, rbserver, rishabh, robee, rokinot, sach1r0, sashik_eth, scaraven, simon135, slywaters
44.3241 USDC - $44.32
https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L473 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L495
Swivel.scheduleFeeChange(), Swivel.setFee() wouldn't work as expected for user preference.
Users can't react properly after ScheduleFeeChange() event because they don't know whether the new fee settings would be better/worse for them.
According to this explanation, these functions are to ensure users can feel comfortable.
Btw with Swivel.scheduleFeeChange(), it emits only when to change fee settings without detailed values.
So users don't know whether the new fee settings will be better or worse for them.
Even if the admin is going to set larger feenominators for lower fee percent, users don't know that until actual fees are set using setFee() and such delays are almost meaningless for users.
I think we should announce the detailed fee settings with Swivel.scheduleFeeChange() function so that users can react accordingly after checking new fee settings.
Solidity Visual Developer of VSCode
Recommend adding an additional array - pendingFeenominators here.
uint16[4] public pendingFeenominators;
And scheduleFeeChange() function should have i, d parameters same as current setFee() function so that pendingFeenominators save new settings. (Also keep original fee settings if some indexs aren't updated.)
After that, we can call setFee() without any params and feenominators will be replaced with pendingFeenominators.
#0 - JTraversa
2022-07-19T00:05:52Z
I probably wouldnt quite call this medium risk and is more low / close to QA though I think the warden's claims are pretty accurate.
We'll likely add the suggested implementation and add it to the event.
#1 - bghughes
2022-08-03T01:21:06Z
I probably wouldnt quite call this medium risk and is more low / close to QA though I think the warden's claims are pretty accurate.
We'll likely add the suggested implementation and add it to the event.
I agree that this should be QA in the scope of this contest:
With all this established, we are likely contesting / rejecting most admin centralization issues, unless there are remediations which do not break the ethos of our early / safeguarded launch.
#2 - bghughes
2022-08-03T01:22:19Z
This warden did not submit a QA report so this will act as their QA report
#3 - robrobbins
2022-08-26T22:31:34Z
hmm. i think we could emit the proposed feenominators and not need to store another feenominators array, going to look at this
#4 - robrobbins
2022-08-29T22:41:25Z