code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

Interfaces are not allowed to define any functions while abstract contracts can have a few defined functions (with at least one undefined function). The code base uses interface notations (IContractName in interfaces directory) for what are really abstract contracts.

For example, ISyntheticToken is an abstract contract and not an interface. This allows defining functions in it that accidentally allow critical functionality available to users which is risky. This also affects readability/maintainability because being an abstract contract allows it to derive from ERC20PresetMinterPauser which is transitively inherited here as well.

Proof of Concept

https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/interfaces/ISyntheticToken.sol#L12

https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/interfaces/ILongShort.sol#L5

https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/interfaces/IStaker.sol#L5

https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/interfaces/IYieldManager.sol#L6

https://github.com/code-423n4/2021-08-floatcapital/blob/bd419abf68e775103df6e40d8f0e8d40156c2f81/contracts/contracts/interfaces/ISyntheticToken.sol#L12

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider changing the abstract contracts to interfaces and if that’s possible remove the interface notations for clarity.

JasoonS commented 3 years ago

Fair analysis, we used abstract contracts to prevent inheritance complications.

Example there are inheritance issues if we make IFloatToken an interface rather than abstract since we override the mint function.

We have switched to strictly using interfaces, however we believe this is 0 non-critical

JasoonS commented 3 years ago

With the latest changes we have made to our synthetic tokens it is possible to easily make these into interfaces.

0xean commented 3 years ago

based on

0 — Non-critical: Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas-optimisations.
1 — Low: Low: Assets are not at risk. State handling, function incorrect as to spec, issues with comments.
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

I am inclined to agree with sponsor that this is a sev 0. Downgrading.

code-423n4 / 2021-08-floatcapital-findings