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: 12/78
Findings: 2
Award: $1,049.91
🌟 Selected for report: 1
🚀 Solo Findings: 0
1005.6038 USDC - $1,005.60
https://github.com/code-423n4/2022-07-swivel/blob/main/Marketplace/MarketPlace.sol#L77 https://github.com/code-423n4/2022-07-swivel/blob/main/Creator/Creator.sol#L41 https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/VaultTracker.sol#L32
MarketPlace.createMarket() calls Creator.create() which creates an instance of ZcToken and a VaultTracker. VaultTracker takes msg.sender as the admin. We know that if contract A calls contract B which calls contract C, msg.sender in contract C is the address of B i.e. the msg.sender in VaultTracker is the address of the creator contract. However, the creator contract is not able (and not supposed to) interact with the VaultTracker unlike the marketplace contract.
Manual analysis
Modify the constructor of the VaultTracker contract so that the creator contract can pass in msg.sender (MarketPlace’s address) to be used as admin.
#0 - bghughes
2022-08-04T23:20:14Z
Using this as the main admin constructor issue. Given it is admin related, I believe going with the Medium issue instead of #134 makes sense.
#1 - robrobbins
2022-08-09T17:18:22Z
see #134
🌟 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.3101 USDC - $44.31
https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L162 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L195 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L323 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L358
premiumFilled = (a * o.premium) / o.principal is called in 4 different functions. These functions are initiateZcTokenFillingVaultInitiate, initiateZcTokenFillingZcTokenExit, exitVaultFillingVaultInitiate and exitVaultFillingZcTokenExit
In the following scenarios, assume that taker specifies a such that a * o.premium / o.principal rounds down to 0. This is quite a common secnario e.g. when a taker wants to fill a small part of an order.
initiateZcTokenFillingVaultInitiate: If premiumFilled is 0, it means that the maker is not paying any premiums to the taker. The zc and vault positions are created and the vault positions are transferred to the maker for free. No fees will be paid to Swivel as well.initiateZcTokenFillingZcTokenExit: If premiumFilled is 0, it means that the taker is transferring more underlying assets assets to the maker since no premiums are paid to the taker. No fees will be paid to Swivel as well.exitVaultFillingVaultInitiate: If premiumFilled is 0, it means that the maker is not paying any premium to the taker but the vault positions are transferred from the taker to the maker. No fees will be paid to Swivel as well.exitVaultFillingZcTokenExit: If premiumFilled is 0, it means that Swivel overpays the maker. The zc and vault positions are burnt but the taker is not paid anything (effectively given to the maker).Manual analysis
Require that premiumFilled cannot be 0.
#0 - JTraversa
2022-07-15T23:04:59Z
The precision lost is likely not worth a report, and the use case of transacting minor amounts probably overstated (given it would cost more in gas to do so than the value transacted in most cases)?
Users only fill orders where the principal and premium are comparable (e.g. a 1% yield requires 2 digits of precision difference) meaning the only case where the premiumAmount could be 0 is when amount is < 1000.
Even with USDC, this would equate to an order of .001 USDC or less, while our exchange's frontend requires orders of at least 2500 USDC to begin with.
#1 - bghughes
2022-08-03T01:33:41Z
Given this is an edge case that could break expected functionality and occur, I believe it should be Medium risk and serve as the main issue with respect to handling the case in which premiumFilled == 0. It is unlikely to occur in practice but seems like a helpful check to have in place at the least.
Downgrading to Medium.
#2 - JTraversa
2022-08-05T00:48:28Z
Just curious in what context would this occur / can you give a reasonable example?
It appears that the report is an issue of someone is irrationally spending ~$5-40 in gas to lend (at worse with USDC's 6 decimals) $0.001, ~.01% of the value spent on the transaction, or in the case of most integrations, ~.000000000000001% of the value spent on gas.
Plus outside that non-rational/self-harming action, this is the same report as someone that is asking us for input sanitization to require (a < 1000), which is explicitly out of scope. (e.g. The suggested implementation has us socialize costs for input sanitization of amounts less than 1000, in the same way direct input sanitization would)
Plus folks might be surprised, but most DeFi protocols have similar amounts of precision loss, something we have become quite familiar with alongside these integrations and the LibCompound and LibRari libraries hah.
#3 - 0xean
2022-08-25T12:17:11Z
I am going to downgrade further to QA. The issue is valid, but amounts to essentially rounding errors when the amounts being transacted are already too small to be practical.
The warden example states this as the external requirement
In the following scenarios, assume that taker specifies a such that a * o.premium / o.principal rounds down to 0. This is quite a common secnario e.g. when a taker wants to fill a small part of an order.
This is not quite common. a would need to a very small dust amount given the decimals of tokens involved.
#4 - robrobbins
2022-08-31T00:43:08Z
i would say it not only uncommon, but approaching pointless.