or
or
By clicking below, you agree to our terms of service.
New to HackMD? Sign up
Syntax | Example | Reference | |
---|---|---|---|
# Header | Header | 基本排版 | |
- Unordered List |
|
||
1. Ordered List |
|
||
- [ ] Todo List |
|
||
> Blockquote | Blockquote |
||
**Bold font** | Bold font | ||
*Italics font* | Italics font | ||
~~Strikethrough~~ | |||
19^th^ | 19th | ||
H~2~O | H2O | ||
++Inserted text++ | Inserted text | ||
==Marked text== | Marked text | ||
[link text](https:// "title") | Link | ||
 | Image | ||
`Code` | Code |
在筆記中貼入程式碼 | |
```javascript var i = 0; ``` |
|
||
:smile: | ![]() |
Emoji list | |
{%youtube youtube_id %} | Externals | ||
$L^aT_eX$ | LaTeX | ||
:::info This is a alert area. ::: |
This is a alert area. |
On a scale of 0-10, how likely is it that you would recommend HackMD to your friends, family or business associates?
Please give us some advice and help us improve HackMD.
Do you want to remove this version name and description?
Syncing
xxxxxxxxxx
Introduction
A security review of the Oxen Blockchain smart contract was done by Cipher Seluths team, with a focus on the security aspects of the smart contracts implementation.
Disclaimer
A smart contract security review can never verify the complete absence of vulnerabilities. This is a time, resource and expertise bound effort where we try to find as many vulnerabilities as possible. we can not guarantee 100% security after the review or even if the review will find any problems with your smart contracts. Subsequent security reviews, bug bounty programs and on-chain monitoring are strongly recommended.
About Cipher Seluths
Cipher Seluths is team of security researchers Udsen & Viraz who have a good experience participating in codearena contests both solo and as a team & have found multiple vulnerabilities in various protocols.
About Oxen Blockchain
The Oxen blockchain is a private payments system that enables the creation of many privacy-preserving applications
The Oxen blockchain and the $OXEN token are at the heart of Oxen. Together they bring privacy, incentivisation, and decentralisation to the Oxen Blockchain tech stack.
Severity classification
Impact - the technical, economic and reputation damage of a successful attack
Likelihood - the chance that a particular vulnerability gets discovered and exploited
Severity - the overall criticality of the risk
Security Assessment Summary
review commit hash - e3f6bdc4eef75bce44b44e076cae0e6163b046e4
Detailed Findings
H-01 Attacker can enable DOS by front running the claiming of network rewards by the user
Lines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L128
Vulnerability details
Impact
The user managing the service nodes can be front run by an attacker resulting in them unable to claim less rewards/no reward or their claim tx being reverted
Proof of Concept
The
updateRewardsBalance
method is used to set the rewards that can be claimable by the user managing a particular serivice node and then the user can callclaimRewards
method to claim the rewardsThere is a BLS signature verification that happens before the reward amount is updated
The issue is in the signed message
msg.sender
isn't checked and there is no mechanism for signature expiry as well which means a attacker can re-use the same sig the user used and set the reward amount as 0 or a lower amount then set by the user before and hence create DOS when a user tries to claim rewardsHere is an example which demonstrates the same
Tools Used
manual review
Recommended Mitigation Steps
change
bytes memory encodedMessage = abi.encodePacked(rewardTag, recipientAddress, recipientAmount);
tobytes memory encodedMessage = abi.encodePacked(rewardTag, msg.sender, recipientAmount);
so the signature cannot be used by someone else and also add a mapping to ensure 1 signature can only be used onceM-01 A whale can get the control of the protocol by having a mojority of stake in the network
Lines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L191
Vulnerability details
Impact
A single whale can stake large amount of session tokens and create majority of the ServiceNodes thus controlling the key functionalities of the protocol where majority of the ServiceNode signatures are needed for execution and hence create a DOS situtation
Proof of Concept
The
ServiceNodeRewards
contract allows any user to createany number of ServiceNodes
to the protocol by adding theBLSPublicKey
by calling theServiceNodeRewards.addBLSPublicKey
function with proof of possession BLS signature .But the issue here is that a single whale can stake large amount of session tokens and create majority of the ServiceNodes thus controlling the key functionalities of the protocol where majority of the ServiceNode signatures are needed for execution (Such as
removeBLSPublicKeyWithSignature
andliquidateBLSPublicKeyWithSignature
).This issue is prevalent specially at the early stages of the protocol where lot of ServiceNodes are not added into the protocol. For example if there are
100 nodes
theblsNonSignerThreshold
will be33
. Hence if a single whale user can have the ownership of67 nodes
he can control the vital operations of the network.When a normal user tries to add more nodes to the network by calling the
addBLSPublicKey
function the whale user canfront run
the sametransaction
with thesame pubkey
andsame signature
and add that node as if it was his. The signature does not have themsg.sender
and hence this is possible. So when thenormal user's transaction
is executed the following condition in theServiceNodeRewards._addBLSPublicKey
will revert.The whale can later
remove
this node from the network and get hisfunds back
by calling theremoveBLSPublicKeyWithSignature
. As a result the attacker can get the control of theServiceNodeRewards
contract since he can single handedly performupdateRewardBalance
,Node removal
,Node liquidation
and cangrief
other users from performing the same operations as well.Further this could
grief the other users
fromremoving their nodes
from the protocol and getting thierstaked amount
andclaimable rewards
from the protocol which isloss of funds
to the other stakers.Tools Used
manual review
Recommended Mitigation Steps
Hence it is recommended to
limit the number of ServiceNodes
asingle user (msg.sender)
canadd
to the protocol by introducing a upperbound per single address and maintaining a mapping ofaddress->uint256
.Or even this upperbound can be a percentage of the total nodes (totalNodes) of the network at a given time.
In addition
Tokenomics
can be planned in a way, to ensure the cost of a node is sufficiently high so that whales can't achieve 300+ nodes easily.M-02 pool liquidation share is rounded down
Lines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L357
Vulnerability details
Impact
The network get's less liquidation fee than it should
Proof of Concept
Usually during a liquidation event, the liquidation fee is calculated in favour of the protocol i.e the calculation result for the protocol's share is always rounded up, but here the result is rounded down since solidity always rounds the result down during division hence the liquidation share of the netowrk is reduced
Tools Used
manual review
Recommended Mitigation Steps
The calculation of the
protocol share
should be such that theprotocol share
should be rounded up.This can be done by using aceil
method of anymath library
.M-03 remove BLS public key check is inconsistent
Lines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L296
Vulnerability details
Impact
bls public key can be removed when
block.timestamp == leaveRequestTimestamp
Proof of Concept
The comment on top of
removeBLSPublicKeyAfterWaitTime
states thatRemoves a BLS public key after a specified wait time
but the check where the current timestamp is same asleaveRequestTimestamp
the transaction does not revert breaking the invaraint of the netowrkTools Used
manual review
Recommended Mitigation Steps
change
if(block.timestamp < timestamp) revert LeaveRequestTooEarly(serviceNodeID, timestamp, block.timestamp);
toif(block.timestamp <= timestamp) revert LeaveRequestTooEarly(serviceNodeID, timestamp, block.timestamp);
Quality Assuarance
L-01 Missing pause functionality in the
ServiceNodeRewards
contractImpact
The network can
remove
the ServiceNode via a liquidation. But if the liquidation is done to decommission fradulent nodes in the event of an attack, theattacker
caneffect
theoperations
of the protocol till he is decommissioned.Tools Used
Manual review
Recommended Mitigation Steps
It is recommended to add the
pause functionality
to the contract topause
addition
of the BLS pub keys (ServiceNodes),removal
of it (in the event of an attack) andclaim reward
operations, as an additional security measureL-02 Attacker can enable DOS by preventing a user adding a service node again
Lines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L191
Vulnerability details
Impact
The user managing a service node can be front run by an attacker if a service node added by the user is liquidated, then the attacker can use the same public key and hence add the node again, preventing the user from adding it again
Proof of Concept
The
_addBLSPublicKey
method is used to add a new service node, for the proof of possession verification we seewhere the
pubkey
is derived hereBN256G1.G1Point memory pubkey = BN256G1.G1Point(pkX, pkY);
andpkX
&pkY
are function parameters which means that the pub key can reused by anyone since there is no check on the msg.sender during proof of possession verificationSo consider a scenario where a user adds a new service node and it get's liquidated after some time, now a attacker can add the node again by passing the same
pkX
&pkY
values and this prevents the user from using their own pubkey and adding the node againTools Used
manual review
Recommended Mitigation Steps
change
BN256G2.G2Point memory Hm = BN256G2.hashToG2(BN256G2.hashToField(string(abi.encodePacked(proofOfPossessionTag, pubkey.X, pubkey.Y))));
toBN256G2.G2Point memory Hm = BN256G2.hashToG2(BN256G2.hashToField(string(abi.encodePacked(proofOfPossessionTag, msg.sender, pubkey.X, pubkey.Y))));
so the signature cannot be used by someone else and also add a mapping to ensure 1 signature can only be used onceL-03
leaveRequestTimestamp
is not checked if it is set or not whenremoveBLSPublicKeyWithSignature
is calledLines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L273
Vulnerability details
Impact
users can bypass the
initiateRemoveBLSPublicKey
method when removing their code by callingremoveBLSPublicKeyWithSignature
directlyProof of Concept
initiateRemoveBLSPublicKey
has a comment which states thatShould be called first and later once the network is happy for node to exist the user should call
removeBLSPublicKeyWithSignaturewith a valid BLS signature returned by the network
and henceleaveRequestTimestamp
is set therethe issue is that in
removeBLSPublicKeyWithSignature
methodleaveRequestTimestamp
is not checked so the users essentially can by pass theinitiateRemoveBLSPublicKey
method which breaks the protocol invariantTools Used
manual review
Recommended Mitigation Steps
add a check in
removeBLSPublicKeyWithSignature
L-04 Recommended to use
openzeppelin Ownable2Step
in place ofOwnable
Lines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L6
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L12
Impact
Here the single-step ownership transfer pattern is used. If by mistake the current owner provides an incorrect address as the new owner when calling the
Ownable.transferOwnership
function, then none of theonlyOwner
methods (seedPublicKeyList, start) of theServiceNodeRewards
contract will be callable again. The recommended solution will be to use thetwo-step ownership transfer
pattern. Using the two-step process thenew owner
will have tofirst claim the ownership
and then theownership
will betransferred to him
.Tools Used
Manual review
Recommended Mitigation Steps
It is recommended to use
Ownable2Step
of OpenZeppelin and use thetwo-step ownership transfer
pattern.L-05
renounceOwnership
function in the inherited openzeppelinOwnable.sol
contract should be overridden to revert if calledImpact
The Ownable.sol has the
renounceOwnership()
function which iscallable
by theowner
torenounce
theownership
of the contract toaddress(0)
as shown below.This is a
virtual
function and it isrecommended
tooverwrite
this function in theServiceNodeRewards.sol
contract such that therenounceOwnership
willrevert
ifcalled
. Hence the Owner is not able to renounce the ownerhsip of the contract to address(0).Tools Used
Manual review
Recommended Mitigation Steps
Recommended to override the
renounceOwnership
function inside theServiceNodeRewards
contract such that the transaction will revert if theowner
calls therenounceOwnership
function by mistake or intentionally tosabotage
the contract.L-06
sumAmounts
variable defined and computed in theServiceNodeRewards.seedPublicKeyList
function is not usedLines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L367
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L382
Impact
The
ServiceNodeRewards.seedPublicKeyList
function is used to add a list of nodes that are already acting as service nodes in the protocol. And thestaked amounts
for each of the nodes to be added is passed in to the function viauint256[] calldata amounts
input array.Then each of these nodes are added to the
serviceNodes
linked list and the respectiveamounts
are added to the local variablesumAmounts
as shown below:But this
sumAmounts
variable is never used within the scope of the function. The initial assumption was thesumAmounts
was calculated to transfer the staked amount to theServiceNodeRewards
contract after theseedList
of nodes are added to theserviceNodes
linked list, at the end of theServiceNodeRewards.seedPublicKeyList
function.But since the staked amount transfer for
seedPublicKeyList
nodes are handled by thefoundation
itself currently thissumAmounts
variable is redundant.L-07 Redundant check
serviceNodeRecipient == address(0)
can be omitted in theServiceNodeRewards._initiateRemoveBLSPublicKey
functionLines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L249-L251
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L259-L260
Impact
In the
ServiceNodeRewards._initiateRemoveBLSPublicKey
function the following check is redundant,Due to following reasons:
The
_initiateRemoveBLSPublicKey
is called in theinitiateRemoveBLSPublicKey
function, as follows with therecipient == msg.sender
and themsg.sender != 0
.And the check
serviceNodeRecipient != recipient
in the_initiateRemoveBLSPublicKey
after theserviceNodeRecipient == address(0)
check, ensures that theserviceNodeRecipient !=0
since therecipient != 0
as explained above.And the
_initiateRemoveBLSPublicKey
is aninternal function
which is called onlyonce
in the ServiceNodeRewards contract and this contract isnot inherited
by any other contract.Tools Used
Manual review
Recommended Mitigation Steps
Hence it is recommended to remvoe the redundant check
serviceNodeRecipient == address(0)
in theServiceNodeRewards._initiateRemoveBLSPublicKey
since it is anyway checked in the subsequent check.L-08 The
ServiceNodeRewards.seedPublicKeyList()
function should only be called before thehardfork
, but no logic implementation to ensure thatLines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L360
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L364-L403
Impact
The
natspec
comments for theServiceNodeRewards.seedPublicKeyList()
function states the following:But there is
no logic implementation
in theseedPublicKeyList
function toensure
that this function canonly be called before
thehardfork
. Hence theseedPublicKeyList
function can be called after thehardfork
to add moreServiceNodes
to the protocol by thefoundation
.Tools Used
Manual review
Recommended Mitigation Steps
Hence it is recommended to configure a logic in the
seedPublicKeyList()
function to ensure that the function can only be called before the hardfork by the foundation. Any call after the hardfork to theseedPublicKeyList()
function should revert which is the intended behaviour of the function aligning with the natspec comment.L-09 The
totalNodes
state variable is not incremented inside thefor loop
of theServiceNodeRewards.seedPublicKeyList
functionLines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L401
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L371-L396
Impact
The
ServiceNodeRewards.seedPublicKeyList
function is used by thefoundation
to add a list of nodes that arealready acting
asservice nodes
in theprotocol
.In the
logic implementation
of theseedPublicKeyList
function theSeed ServiceNodes
are added one by one iterating through afor loop
.But the issue is the
totalNodes
state variable is not incremented inside thefor loop
but only increments after thefor loop
ends. But theupdateServiceNodesLength
function implementation suggests that thetotalNodes = serviceNodesLength()
which means the number of service nodes added to the protocol is equal to the length of theserviceNodes
linked list.As a result irrespecitve of number of
Seed ServiceNodes
added by the foundation via theseedPublicKeyList
function thetotalNodes = 1
after the execution which is erroneous and a broken state.Tools Used
Manual review
Recommended Mitigation Steps
Hence it is recommended to increment the
totalNodes
inside thefor loop
which is used to addSeed ServiceNodes
in theServiceNodeRewards.seedPublicKeyList
function.Informational
I-01 Typo in the natspec comment
Lines of code
https://github.com/oxen-io/eth-sn-contracts/blob/master/contracts/ServiceNodeRewards.sol#L247
Impact
There is a type in the following natspec comment in the
exist
word and should beexit
instead.Tools Used
Manual review
Recommended Mitigation Steps
Typo exist in the above mentioned natspec comment should be corrected as
exit
. Proper natspec commenting will improve the code readability and understanding for the developers and auditors.Gas Optimisations
G-01 Use compiler version >= 0.8.22 or use
unchecked
in for loopsThe for loops used have the counter operation together with the counter initialisation which costs extra gas everytime there is a counter increment so it is recomended to either use a compiler version >= 0.8.22 or do the counter operation in the unchecked block i.e
G-02 Variables only initialized in the constructor should be marked as immutable
stakingRequirement, liquidatorRewardRatio, poolShareOfLiquidationRatio & recipientRatio
are only initialized in the constructor and not updated anywhere else so these variables should be marked as immutable to save gasOther Suggestions
We strongly recommend that the test coverage of the contracts should be close to 100%, static analysers like slither should also be used for marking and fixing any issues identified there.