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: 7/16
Findings: 3
Award: $2,509.67
🌟 Selected for report: 6
🚀 Solo Findings: 0
1053.3363 USDC - $1,053.34
0xRajeev
Owner/admin only functions that change critical parameters should emit events and have timelocks. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.
Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.
There are owner/admin functions that do not emit any events in LongShort.sol. It is not apparent that any owner/admin functions will have timelocks.
See similar High-severity H03 finding OpenZeppelin’s Audit of Audius (https://blog.openzeppelin.com/audius-contracts-audit/#high) and Medium-severity M01 finding OpenZeppelin’s Audit of UMA Phase 4 (https://blog.openzeppelin.com/uma-audit-phase-4/)
Missing events:
Desirable timelock: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/Staker.sol#L221-L224
Manual Analysis
Add events to all owner/admin functions that change critical parameters. Add timelocks to introduce time delays for critical parameter changes that significantly impact market/user incentives/security.
#0 - JasoonS
2021-08-12T09:43:47Z
We will manage timelocks and multi-sigs externally to these contracts.
#1 - JasoonS
2021-08-13T20:07:01Z
I would consider this a duplicate of #84 in many ways. (or at least #84 is a sub-issue of this issue)
#2 - 0xean
2021-08-24T23:46:17Z
duplicate of #84 as both offer solutions for dealing with privileged functionality (including the transfer of ownership). Leaving severity as 2 based on the potential risks associated with an incorrect admin change or similar.
142.2004 USDC - $142.20
0xRajeev
Zero-address checks are a best-practice for input validation of address parameters. There are many places where there is missing.
Manual Analysis
Add zero-address checks.
#0 - JasoonS
2021-08-12T09:38:15Z
Duplicate #1
351.1121 USDC - $351.11
0xRajeev
The code comment says: “// The exponent has to be less than 5 in these versions of the contracts.” but the code immediately after the comment implements a check “< 6.” It is unclear if the comment is incorrect or the check is wrong. An incorrect check may have mathematical implications.
Manual Analysis
Revisit comment and code to sync them by fixing the comment or the code whichever is incorrect.
#0 - JasoonS
2021-08-12T10:14:13Z
Thanks - has been pointed out before. 0 non-critical
#1 - 0xean
2021-08-25T01:29:09Z
Per https://docs.code4rena.com/roles/wardens/judging-criteria#estimating-risk-tl-dr - comments are a 1 (Low Risk). Agreeing with the warden here.
351.1121 USDC - $351.11
0xRajeev
Race-condition risk with initialize functions if deployment script is not robust to create and initialize contracts atomically or if factory contracts do not create and initialize appropriately.
If this is not implemented correctly, an attacker can front-run to initialize contracts with their parameters. This, if noticed, will require a redeployment of contracts resulting in potential DoS and reputational damage.
Manual Analysis
Ensure deployment script is robust to create and initialize contracts atomically or factory contracts create and initialize appropriately.
#0 - JasoonS
2021-08-12T09:32:27Z
We use open-zeppelin scripts todo this automatically.
Additionally we initialize the base implementations too to prevent any foul play by pranksters.
#1 - 0xean
2021-08-26T11:54:21Z
Given that the contest didn't include the scope of the scripts and that this is a risk in the contract implementation without a factory I believe this is a valid risk even if the sponsor believes its mitigated.
🌟 Selected for report: 0xRajeev
249.7566 USDC - $249.76
0xRajeev
There are multiple places where state variables are reused within functions by loading them multiple times. These operations result in expensive SLOAD instructions where the first SLOAD costs 2100 gas and successive SLOADs of the same variable cost 100 gas (since the Berlin hardfork).
Using local memory variables to cache them will remove the unnecessary SLOADs costing 100 gas resulting in MLOADs that only cost 3 gas units.
Caching latestMarket can save upto 13 SLOADs i.e. 1300 gas: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/LongShort.sol#L261-L298
Caching staked can save upto 1 SLOAD i.e. 100 gas: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/LongShort.sol#L267 https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/LongShort.sol#L276
Caching latestMarket can save upto 3 SLOADs i.e. 3 gas: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/LongShort.sol#L352-L365
Caching marketUpdateIndex[marketIndex] appropriately can save many SLOADs: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/LongShort.sol#L817-L819
Caching longShort address can save 300 300 gas by avoiding 3 SLOADs: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/TokenFactory.sol#L68-L70
Caching amountReservedInCaseOfInsufficientAaveLiquidity can save upto 2 SLOADs i.e. 200 gas: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/YieldManagerAave.sol#L114-L121
Caching paymentToken can save upto 1 SLOAD i.e. 100 gas: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/YieldManagerAave.sol#L132-L142
Caching aaveIncentiveController can save upto 1 SLOAD i.e. 100 gas: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/YieldManagerAave.sol#L162-L167
Caching totalReservedForTreasury can save upto 1 SLOAD i.e. 100 gas: https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/YieldManagerAave.sol#L162-L167
Manual Analysis
Cache storage-based state variables in local memory-based variables appropriately to convert SLOADs to MLOADs and reduce gas consumption from 100 units to 3 units.
#0 - JasoonS
2021-08-12T09:14:24Z
Thank you!
(TODO, group with other 'local variable rather than storage' gas optimizations)
#1 - JasoonS
2021-08-13T19:33:35Z
Quite a few of these are sadly not possible due to stack to deep issues.
Others aren't worth it due to being only applicable in extremely rare edge cases (eg: code-423n4/2021-08-floatcapital@bd419ab/contracts/contracts/YieldManagerAave.sol#L114-L121 - aren't anticipating aave to ever not have liquidity) - just makes code harder to read and adds to the stack unnecessarily.
🌟 Selected for report: 0xRajeev
249.7566 USDC - $249.76
0xRajeev
Named returns are ignored in several places to favor explicit returns. Removing such named return variables can optimize a little gas and improve readability.
Manual Analysis
Remove unused named return variables.
#0 - JasoonS
2021-08-12T09:20:09Z
Thank you.
I didn't think about gas implications, but makes sense.
We have some codegen stuff that uses the return value name which is quite nice.
112.3905 USDC - $112.39
0xRajeev
Changing a function’s visibility from public to external can save gas by avoiding the unnecessary copying of data to memory. Function stakeFromUser() in Staker.sol is only called from SyntheticTokens.sol and not from within the contract itself which means this can be made external.
Manual Analysis
Change function visibility to external where possible.
#0 - JasoonS
2021-08-12T10:17:33Z
Thanks