Platform: Code4rena
Start Date: 05/08/2021
Pot Size: $50,000 USDC
Total HM: 9
Participants: 16
Period: 7 days
Judge: 0xean
Total Solo HM: 4
Id: 22
League: ETH
Rank: 5/16
Findings: 2
Award: $1,975.79
🌟 Selected for report: 2
🚀 Solo Findings: 0
1053.3363 USDC - $1,053.34
tensors
Inputting the wrong address here could lock out a lot of the funds and smart contract methods.
Require the changed address to confirm the switch (with a pendingAdmin, pendingTreasury variable.
#0 - JasoonS
2021-08-10T12:30:44Z
0 - non-critical
Those functions are onlyAdmin.
Additionally, the whole LongShort contract is upgradable, so even worst case isn't an issue.
Introducing a mechanism is more likely to introduce an unintended bug than to just leave the code as is.
Something like https://eips.ethereum.org/EIPS/eip-165 is also overkill for this case.
#1 - moose-code
2021-08-11T10:22:32Z
Agree with Jasoons assessment
#2 - Stentonian
2021-08-11T10:30:29Z
Also agree
#3 - 0xean
2021-08-25T01:16:31Z
duplicate of #85
142.2004 USDC - $142.20
tensors
Many functions throughout LongShort.sol and YieldManager.sol have no simple checks for validating inputs. Below some examples are linked.
https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/LongShort.sol#L254 https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/YieldManagerAave.sol#L149
Simple validations like requiring non-zero address or checking that amounts are non-zero would fix this.
#0 - JasoonS
2021-08-10T12:25:20Z
0 - non-critical
Both examples given are not vulnerabilities since they are not public functions.
This is an onlyAdmin function. We purposely removed null checks from admin functions (we will only call these functions under highly controlled circumstances with via code (ie ethers.js) that checks for forgotten arguments).
This function is longShortOnly so ONLY the LongShort contract.
If you can find a bug (an actual example) in the LongShort.sol contract where it can give the incorrect arguments to the YieldManagerAave contract then this could be a valid issue.
#1 - 0xean
2021-08-25T01:13:50Z
given the downside of not having these checks in place and for example the admin being set to a null address there seems little benefit to not having them. Downgrading to 1
🌟 Selected for report: tensors
780.2491 USDC - $780.25
tensors
Because smart contracts need to be poked to execute, trades placed before an oracle update won't be executed until someone else calls the function to execute queued trades. This means that a bot must run to constantly execute trades after every oracle update.
If such a bot was not running, users would have an incentive to only execute their trades after a favorable oracle update. However, having a dedicated bot run by the team centralizes the project with a single failure point. The typical solution here is to create keeper incentives for the protocol.
Either make sure the team has a bot running or preferably create incentives for other users to constantly keep the queued orders executing.
#0 - JasoonS
2021-08-10T12:36:54Z
0 - non-critical
In the worst case of the bot going down, no vulnerabilities open up.
The only issue that arises is that the price won't be tracked as accurately during those periods (but it will still track it, just not as accurately).
The protocol allows anyone to call an update, and any user interaction (non-update interaction) will also call the update if an update is due.
If such a bot was not running, users would have an incentive to only execute their trades after a favorable oracle update.
That is great, then the incentives are working, they cannot do better than a fair price update which is 'fair' in my opinion.
Indeed: We have been in talks with the chainlink keeprs team to be the first project to use them when they launch on polygon. We should have mentioned this in the README. Until then we have built a robust bot.
Note, this bot is for UX, not to patch up a vulnerability.
#1 - moose-code
2021-08-11T10:41:35Z
Agree with @JasoonS . The bot failing opens no vulnerabilities, the synthetics may just less closely track the underlying if no contract interactions are present.
Given rational markets with actors on both sides (long and short), and given it will always be beneficial for one side to execute the update to capitalize on the movement, its safe to assume that in a rational market the updateSystemState in every case should be called by at least one participant, meaning theoretically a bot isn't even necessary.
#2 - Stentonian
2021-08-11T10:58:16Z
trades placed before an oracle update won't be executed until someone else calls the function to execute queued trades.
What function is this? There are multiple update functions so it's not clear which one the warden is referring to. If they are referring to the function _executeOutstandingNextPriceSettlements (the one that they linked in the 'Proof of concept' section) then the warden's statement is a non-issue. The system is designed to do exactly what the warden says. So the next statement is incorrect:
This means that a bot must run to constantly execute trades after every oracle update.
LongShort keeps track of which trade should happen at which price for each user, so when _executeOutstandingNextPriceSettlements is eventually called (no matter how many price updates have occurred since the trade request) it will use the oracle's next price update that occurred right after the time when trade was requestd. And _executeOutstandingNextPriceSettlements is called before any other action is taken by the user that would require the data from the trade having completed. No need to have a bot call this function.
If the warden is referring to another update function then it's not clear which.
The only issue that arises is that the price won't be tracked as accurately during those periods (but it will still track it, just not as accurately).
This is not actually mentioned by the warden in the issue description, even tho it is a real problem. So I don't think we need to include it here. The warden is specifically pointing out executeOutstandingNextPriceSettlements which will never have any issues no matter if there is a bot or not.
#3 - 0xean
2021-08-26T11:28:02Z
There are incentives built into the market already for users to update if the bot was to stop running. Yes, the bot should be treated as a tier-1 service for a reasonable user experience and for the most efficient tracking, but the risk of this to the system as a whole locking user funds or allowing people to significantly and unfairly profit doesn't seem to be there.
All that being said, it is a failure point within the system but one with low risk, downgrading to 1 - Low Risk.