Audit done by saksham
https://github.com/CallistoSecurity/EnduracoinToken/tree/main
In the EnduraCoinToken contract pause functionality has been integrated using the OZ's pausable library , and pause and unpause methods have been introduced. But all the critical functions lack the check to see if the contract is in paused state or not , for example mint can be called to mint ENDC even when contract is paused.
https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L31 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L37 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L41
In the EnduracoinValue contract here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L13
we calculate the uptime , but this time would be incorrect as , for example if 18.9 days have passed since the startTime , uptime
would be 18.
Due to this when we calculate timestampOfLastVGAChange here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L27
, we will get the timestamp of startDate + 18 days converted to seconds , while the correct timestamp should have been 18.9 + startDate.
Use simple block.timestamp instead
Let's assume setDailyValueGainAdjustment is called , now timestampOfLastVGAChange is set to today.
Now if the function is called again , but within 24 hrs , baseValue calculated here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L44
would be unchanged (((block.timestamp - timestampOfLastVGAChange) / 86400) would be 0) AND the value would of VGA be updated to the new value.
Add a timlock for 1 day for setDailyValueGainAdjustment
When we call setDailyValueGainAdjustment to update the value of dailyValueGain , it calls updateValue here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L26
which recalculates baseValue here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L44 , which updates it according to the time passed between two updates.
But if let's say 18.9 days have passed between the two updates , the baseValue would be adjusted by 18 days (due to rounding) and the resulting
value would be lesser.
Similar rounding happens in getCurrentValue() here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L53
Use block.timestamp instead
We calculate current value here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L51 , if block.timestamp - timestampOfLastVGAChange) / 86400) == 0 meaning time elapsed since last VGA change is within a day , it returns
baseValue + dailyValueGain , which is wrong . Since 0 days have passed since the last update daily value gain must be 0 , therefore
the returned value should be just baseValue
Assume a case where all 50 Billion of the tokens are minted to other users(through a series of burn and mints) , now the owner or let's say the protocol needs to burn some tokens , but they can't as burn won't work (owner has 0 tokens) and users have not allowed owner to burn. In cases like these users should be able to burn.
Instances:
https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L23 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L27 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L31 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L37 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L41 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L62 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L68 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L123 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L131
In the EnduraCoinToken contract the function beforeTokenTransfer can be removed as it is not used(cause notransfer of tokens is implemented) https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L50
Use scientific notation instead of using decimal representation for readability , example here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinToken.sol#L16
use 5*1e9 ,
https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L14 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L33
Currently the code https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L12 uses 1 Jan 2023 as the start date
which is used to calculate uptime which is used to calculate baseValue. For meaningful results set the startDate to the date when the contract
will be deployed , or set it in the constructor on deployment.
There are a couple of instances where a line of code can be omitted.
Example 1 - https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L28
No need for the above as value is already set here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L45
Example 2 - https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L61
No need for this as we already check this condition here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L33
There is no purpose of having a onlyOwner modifier on view functions , it just hinders decentralisation , consider removing them.
https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L51 https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L56
If the number is big enough , it would result in too much gas consumption in the convertToString function
Here https://github.com/CallistoSecurity/EnduracoinToken/blob/main/EnduracoinValue.sol#L51 at every iteration the length is calculated , this would result in huge gas consumption if the number of bytes are big enough. Consider caching the length of the number of bytes.
Many functions in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security, but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
The codebase is missing a detailed test suite , building a test suite with 100% coverage is essential in the development and testing of a protoco/project.
Rounding Will Give Incorrect uptime Value
Rounding is not an issue here, because uptime is calculated in full days.