Security Audit
June 19, 2023
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for Tales of Elleria's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from June 5, 2023 to June 6, 2023.
The purpose of this audit is to review the source code of certain Tales of Elleria Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.
Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.
The following is an aggregation of issues found by the Macro Audit team:
Severity | Count | Acknowledged | Won't Do | Addressed |
---|---|---|---|---|
High | 1 | - | - | 1 |
Medium | 3 | - | - | 3 |
Low | 7 | - | - | 7 |
Code Quality | 5 | - | - | 5 |
Gas Optimization | 2 | - | - | 2 |
Tales of Elleria was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
28d9648caeebf8e41f7a687d1fb82f78ae5bae68
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
contracts/ElleriaElmBridge.sol |
|
contracts/ElleriumLPStakingPool.sol |
|
contracts/VerifySignature.sol |
|
Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.
Click on an issue to jump to it, or scroll down to see them all.
+=
instead of +
since it costs less gas
require()
/ revert()
statements should have descriptive reason strings
We quantify issues in three parts:
This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:
Severity | Description |
---|---|
(C-x) Critical |
We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost. |
(H-x) High |
We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec. |
(M-x) Medium |
We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner. |
(L-x) Low |
The risk is small, unlikely, or may not relevant to the project in a meaningful way. Whether or not the project wants to develop a fix is up to the goals and needs of the project. |
(Q-x) Code Quality |
The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design. |
(I-x) Informational |
Warnings and things to keep in mind when operating the protocol. No immediate action required. |
(G-x) Gas Optimizations |
The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it. |
In the EllerianHeroUpgradeable
contract, SynchronizeHero()
function may be invoked multiple times with the same signature to reset Hero
related attributes to previously valid but currently obsolete data.
VerifySignature
contract documentation indicates that its verification functions, such as bigVerify()
on which SynchronizeHero()
relies, do not have replay attack protection and that any logic reliant must also have logic guarding against replay attacks.
However, SynchronizeHero()
does not follow this guideline and therefore is vulnerable to replay attack.
Remediations to Consider:
ElleriaElmBridge:RetrieveElleriumFromGame()
using a nonce as part of the signed message.This will be deprecated, and the source of truth is now our backend servers instead to support custom in-game functionality.
The backend servers will be integrated with Chainlink’s VRF to proof state changes to ensure reliability and integrity.
In the ElleriumLPStakingPool
contract, rewardRate
value is denominated in Gwei. In addition, setRewardRate()
, the function for updating rewardRate
expects input denominated in Gwei _emissionRateInGWEI
. Also, in getReward()
function totalRewards[msg.sender]
is multiplied by 1e9
to get the actual reward amount in Wei.
function getReward() public nonReentrant updateReward(msg.sender) {
uint256 reward = totalRewards[msg.sender] * 1e9;
if (reward > 0) {
totalRewards[msg.sender] = 0;
rewardsToken.mint(msg.sender, reward);
emit ClaimedReward(msg.sender, reward);
}
}
However, the implementation of the checkTotalRewards()
function does not have corresponding multiplication by the 1e9
factor, even though the natspec comment indicates that the return value is denominated in Wei.
/// @notice Exposes sender's claimable rewardToken balance.
/// @return Sender's Claimable rewardToken balance in WEI.
function checkTotalRewards() external view returns (uint256) {
return totalRewards[msg.sender];
}
Remediations to Consider:
checkTotalRewards()
so it returns the totalRewards
amount for a particular user denominated in Wei instead of Gwei, orrewardRate
to be expressed in Wei instead of Gwei. Correspondingly, update setRewardRate()
and getReward()
function implementations to work with Wei denomination to prevent accidental issuance of enormous rewards which could happen at the moment if setRewardRate()
is called with an emission rate incorrectly provided in Wei.In the ElleriumLPStakingPool
and ElleriaElmBridge
contracts, setAddresses()
and SetReferences()
functions can be called only by the contract owner. These two functions are responsible for initializing and updating important external contract dependencies, which include staking token, rewards token, staking fee, and signature verification contract.
If these contract dependencies are not properly set or they get accidentally misconfigured, overall system operation will be affected. In addition, the two previously mentioned contracts cannot be properly used after deployment without a call to these functions.
To increase user trust in the overall system, it is important to provide means for users to verify important state changes and to react in case they disagree with the same. Also, it is best practice to minimize important system configuration updates, such as updates to external dependencies, unless they are necessary.
Remediations to consider:
Contracts within scope have been modified to only set dependencies in the constructor. Subsequent contracts that are single-utility will also be switched to follow the same pattern.
In the ElleriumLPStakingPool
and ElleriaElmBridge
contracts, the withdrawToken()
function, which is callable only by the owner, enables the caller to withdraw all ERC20 assets from the contract.
function withdrawToken(address _tokenContract, uint256 _amount) external onlyOwner {
IERC20 tokenContract = IERC20(_tokenContract);
tokenContract.transfer(msg.sender, _amount);
}
While this functionality may serve a purpose in an emergency situation, its presence in its current form may affect users’ trust in the overall system.
Remediations to Consider:
withdrawToken()
from the ElleriaElmBridge
since ElleriaElmBridge
does not store ELM assets but burns them during BridgeElleriumIntoGame()
operation.withdrawToken()
in ElleriumLPStakingPool
with functionality for individual users to perform an emergency withdrawal in case an emergency state is enabled.Resolutiom: withdrawToken() removed from both contracts.
The contract ElleriaElmBridge does not manage ERC20 tokens, hence there was no risk of funds within the contract being mismanaged.
ElleriumLPStakingPool, the intent was to withdraw stuck tokens, though makes sense it can cause trust issues.
The "emergency state" is essentially RewardRate set to 0 and feesTaken set to false.
In the following instances, return values of transfer
are ignored:
ElleriumLPStakingPool:stake()
ElleriumLPStakingPool:unstake()
ElleriumLPStakingPool:withdrawToken()
ElleriaElmBridge:withdrawToken()
ElleriaElmBridge:BridgeElleriumIntoGame()
For handling non-standard ERC20 token implementations which do not revert, but return false on transfer failure, consider using OZ’s SafeERC20 variants of token handling functions safeTransfer()
/ safeTransferFrom()
.
Remediations to Consider:
transfer()
and transferFrom()
, consider using OZ’s SafeERC20 variants safeTransfer()
/ safeTransferFrom()
.Additional updates in commit 50bdf09d1779fd76caeaaaf6d7dbd4fce5320f9b.
The VerifySignature
contract implements signature verification based on the EIP-712 standard.
However, the current implementation does not include DOMAIN_SEPARATOR
as part of the signature. As a result, signatures generated on one chain may be reused on other chains.
Remediations to Consider:
DOMAIN_SEPARATOR
verification functionality.In the ElleriaElmBridge
when RetrieveElleriumFromGame()
is executed corresponding ERC20Withdraw
event is emitted. This event contains info about the caller, ELM token contract, the amount withdrawn, and the current number of withdrawals.
However, the ERC20Withdraw
event does not contain _txnCount
information, which is practically the id
of the withdrawal transaction. As a result, it is harder to determine which previously signed transactions by the internal server have already been processed on-chain.
Remediations to consider:
_txnCount
information to the ERC20Withdraw
event.Additional updates in commit 50bdf09d1779fd76caeaaaf6d7dbd4fce5320f9b.
In the EllerianHeroUpgradeable
contract, SynchronizeHero()
function implementation relies on a fixed set of elements (11) from provided _data
array argument. This _data
argument is also forwarded to VerifySignature:bigVerify()
function which iterates through _data
elements and calculates message hash for signature verification.
However, since the _data
array can be of a length that is greater than the necessary set of elements, the malicious caller of SynchronizeHero()
can add extra elements to the _data
array to generate a hash collision.
Remediations to consider:
SynchronizeHero()
calls with _data
array length greater than required.Function to be deprecated.
In the ElleriumLPStakingPool
and ElleriaElmBridge
contracts, several important state-changing functions miss corresponding event emissions. These are:
ElleriaElmBridge:SetReferences()
ElleriaElmBridge:withdrawToken()
ElleriumLPStakingPool:setAddresses()
ElleriumLPStakingPool:setRewardRate()
ElleriumLPStakingPool:withdrawToken()
However, without appropriate events, it is difficult for off-chain tools to track and monitor on-chain system behavior.
Remediations to consider:
Changed variables for the dependencies to be public.
Contract dependencies set only within the constructor.
In the ElleriumTokenERC20v2
contract, the internal _transfer()
function is meant to handle the transfer to 0 address as a burn operation. When recipient is address(0), totalSupply
is reduced by the provided amount of token.
// Demint for burn transactions.
if (recipient == address(0)) {
_totalSupply = _totalSupply - amount;
}
_balances[actualRecipient] = _balances[actualRecipient] + amount;
However, what differs from expected behavior is that _balance
of address(0) will still get increased by the provided amount. This not be the case since it invalidates the invariant that the sum of individual account balances should be equal to the totalSupply
.
Remediations to Consider:
In the ElleriumTokenERC20v2
contract, the internal _transfer()
function performs token transfer with additional logic to prevent transfers before the system is completely configured. One aspect of this mechanism is that those who attempt transfers beforehand will get blacklisted. A consequence of getting blacklisted is that transfers from the blacklisted address will be rerouted to the owner-controlled address.
When a transfer from the blacklisted address is processed, corresponding balances are updated. However, Transfer event emission references the initial recipient
instead of actualRecipient
which in this edge case is incorrect.
...
_balances[actualRecipient] = _balances[actualRecipient] + amount;
emit Transfer(sender, recipient, amount);
Remediations to Consider:
actualRecipient
instead of the recipient
.In the ElleriumLPStakingPool:unstake()
implementation, the taxFee
transfer call always happens.
stakingToken.transfer(stakingLPFeesAddress, taxFee);
However, this call is unnecessary in the following situations:
applyFees
is falsetaxFee
is 0stakingLPFeesAddress
is address(0)+=
instead of +
since it costs less gas
When updating a value in an array with arithmetic, using array[index] += amount
is cheaper than array[index] = array[index] + amount
. This is because you avoid an additional mload
when the array is stored in memory and a sload
when the array is stored in storage.
This can be applied for any arithmetic operation including +=
, -=
,/=
,*=
,^=
,&=
, %=
, <<=
,>>=
, and >>>=
. This optimization can be particularly significant if the pattern occurs during a loop.
Saves 28 gas for a storage array, 38 for a memory array
Instances (4):
File: ETH_ElleriumTokenERC20.sol
229: _balances[actualRecipient] = _balances[actualRecipient] + amount;
242: _balances[account] = _balances[account] + amount;
File: ElleriumLPStakingPool.sol
129: balances[msg.sender] = balances[msg.sender] + _amount;
144: balances[msg.sender] = balances[msg.sender] - _amount;
Consider using Solidity’s 0.8.4 feature - custom errors to reduce contract size, increase code readability, and have more gas-efficient operations.
Implemented for contracts under scope.
require()
/ revert()
statements should have descriptive reason strings
Instances (3):
File: ETH_ElleriumTokenERC20.sol
61: require (msg.sender == owner() || _approvedAddresses[msg.sender]);
File: ElleriumLPStakingPool.sol
122: require(_amount > 9999);
140: require(_amount <= balances[msg.sender]);
In some contracts, function names are capitalized, while in others, they follow the standard camelCase
naming convention. Consider using the camelCase
convention consistently throughout the whole codebase.
Implemented for contracts under scope, to maintain the standard across all other contracts as well.
In the ElleriumLPStakingPool
same logic for calculating staking taxFee is implemented in two different ways in the following two functions:
getWithdrawalFees()
unstake()
Consider reusing getWithdrawalFees()
in unstake()
instead of reimplementing it.
Changed for contracts under scope.
Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.
Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.
The scope of this report and review is limited to a review of only the code presented by the Tales of Elleria team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.