Tales of Elleria A-1

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:

• Implement replay attack protection similar to how it is implemented in ElleriaElmBridge:RetrieveElleriumFromGame() using a nonce as part of the signed message.

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:

• Update checkTotalRewards() so it returns the totalRewards amount for a particular user denominated in Wei instead of Gwei, or
• Change rewardRate 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:

• Set contract dependencies in the contract constructor during contract deploy, and
• In case contract dependencies updates are required, add a timelock mechanism with appropriate delay when contract dependency updates are performed in an existing system.

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:

• Consider removing withdrawToken() from the ElleriaElmBridge since ElleriaElmBridge does not store ELM assets but burns them during BridgeElleriumIntoGame() operation.
• Consider replacing withdrawToken() in ElleriumLPStakingPool with functionality for individual users to perform an emergency withdrawal in case an emergency state is enabled.

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:

• Instead of IERC20’s transfer() and transferFrom(), consider using OZ’s SafeERC20 variants safeTransfer() / safeTransferFrom().

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:

• Implement the EIP-712 standard with corresponding 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:

• Add _txnCount information to the ERC20Withdraw event.

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:

• Add guard condition to prevent SynchronizeHero() calls with _data array length greater than required.

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:

• To facilitate off-chain tracking and monitoring, introduce appropriate events for important system state changes.

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:

• Update implementation so that the balance update above is placed in the else block.

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:

• Update Transfer event emission to reference 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 false
• taxFee is 0
• stakingLPFeesAddress is address(0)

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.

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.

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.

• Add natspec comments for contract variables
• Add natspec for missing function arguments and return variables
• Add natspec comments for event declarations and their arguments
• (If custom errors are implemented) Add natspec comments for custom errors