Internal Review of SolarSwap
Summary
Scope
Issue Overview
Issue Detail
1. Different versions of Solidity are used
Category Severity Location Status
Coding
Informational contracts/*.sol
Style
Description
Different versions of Solidity are used within the project including: [0.4.18, 0.5.16, 0.6.6, 0.6.12, 0.8.4] .
Recommendation
Consider using ONLY one version.
2. Function documentation is missing
Category Severity Location Status
Coding
Informational contracts/*.sol
Style
Description
Most of the functions and variables lack documentation.
Recommendation
Consider adding documentation for better code review and audit.
3. Variable shoule be constant or must follow variable naming convention
Category Severity Location Status
Coding
Optimization MasterChef.sol#56
Style
Description
The variable MasterChef.BONUS_MULTIPLIER should be declared as a constant to save gas. Should it be changed in the future,
consider using variable naming convention for it.
Recommendation
Add the constant identifier or change the variable name.
4. Function paramters are not in mixedCase
Category Severity Location Status
Coding MasterChef.sol, Treasury.sol, SolarswapFactory.sol,
Informational
Style SolarswapPair.sol
Description
Solidity defined a naming convention that should be followed. For function parameters, they should confront to the mixedCase
format.
Recommendation
Follow the Solidity naming convention.
5. Public variables should not start with an underscore ( _ )
Category Severity Location Status
Coding
Informational ARC20.sol#38, ARC20.sol#40
Style
Description
Public variables _balances and _allowances of ARC20.sol start with an underscore.
Recommendation
Consider removing _ for these variables.
6. Unsafe block.timestamp usage
Category Severity Location Status
Low SolarswapERC20.sol#112, SolarswapPair.sol#110
Description
The use of block.timestamp is considered to be unsafe since it can be manipulated by the block producer (to some extent),
especially for comparisions. If time comparison is crucially needed, consider using block.number instead because this number
is forced to increase one-by-one by the protocol and is much more harder to be manipulated. However, at some degree, use
block.number is not a future proof as the block time may change during forks, etc,.
Recommendation
Consider using block.number as an estimation for time.
7. Lack of zero-check
Category Severity Location Status
Low Treasury.sol#19
Description
The claim function is a critical function that allows the owner of the Treasury to withdraw and amount of ASA from it. If in the
case the back-end of the owner calls this function without initializing the parameter _to , the fund will be lost.
Recommendation
Should add a check for _to != 0x00 .
8. Event missing
Category Severity Location Status
Low MasterChef.sol#95,#118,#269
Description
The function add, set, updateWasaPerBlock functions should have events emitted for better off-chain tracking.
Recommendation
Emit events for add, set, updateWasaPerBlock functions.
9. WASA should not inherit Ownable
Category Severity Location Status
Logic Low WASA.sol#8
Description
The wrapped ASA (a.k.a WASA ) should not be owned by any external account. As WASA inherits from ARC20 which is ownable
by design, WASA is also owned by the address which initiates the contract. This address is able to mint WASA out of thin air via
the mint function. In this case, total supply of WASA might exceed the total supply of ASA .
In reality, WASA.sol can be permissionlessly implemented by locking ASA for minting WASA , and releasing ASA for burning
WASA .
Recommendation
Remove the Ownable inheritance and the mint function.
10. Incorrect _totalSupply of WASA when deposit and withdraw
Category Severity Location Status
Logic High WASA.sol#16, #21
Description
In the deposit and withdraw functions, although the balance of msg.sender is recorded, the _totalSupply of WASA is
never changed.
Recommendation
Consider updating the value of _totalSupply when the functions deposit and withdraw are invoked.
