Omniscia Sovryn Audit

Federation Manual Review Findings

Federation Manual Review Findings

FED-01M: Arbitrary User Data

TypeSeverityLocation
Logical FaultMediumFederation.sol:L101, L125

Description:

The userData argument is not part of a particular transaction vote and thus can be arbitrarily set by the last vote caller.

Example:

sovryn-token-bridge/bridge/contracts/Federation.sol
88function _voteTransaction(
89    address originalTokenAddress,
90    address receiver,
91    uint256 amount,
92    string memory symbol,
93    bytes32 blockHash,
94    bytes32 transactionHash,
95    uint32 logIndex,
96    uint8 decimals,
97    uint256 granularity,
98    bytes memory userData
99) internal onlyMember returns(bool) {
100    // solium-disable-next-line max-len
101    bytes32 transactionId = getTransactionId(originalTokenAddress, receiver, amount, symbol, blockHash, transactionHash, logIndex, decimals, granularity);
102    if (processed[transactionId])
103        return true;
104

105    if (votes[transactionId][_msgSender()])
106        return true;
107

108    votes[transactionId][_msgSender()] = true;
109    // solium-disable-next-line max-len
110    emit Voted(_msgSender(), transactionId, originalTokenAddress, receiver, amount, symbol, blockHash, transactionHash, logIndex, decimals, granularity);
111

112    uint transactionCount = getTransactionCount(transactionId);
113    if (transactionCount >= required && transactionCount >= members.length / 2 + 1) {
114        processed[transactionId] = true;
115        bool acceptTransfer = bridge.acceptTransferAt(
116            originalTokenAddress,
117            receiver,
118            amount,
119            symbol,
120            blockHash,
121            transactionHash,
122            logIndex,
123            decimals,
124            granularity,
125            userData
126        );
127        require(acceptTransfer, "Federation: Bridge acceptTransfer error");
128        emit Executed(transactionId);
129        return true;
130    }
131

132    return true;
133}

Recommendation:

We strongly recommend it to be factored in the transaction ID as otherwise it is prone to manipulation by a single party.

Alleviation:

A change was introduced whereby a secondary transaction ID is calculated using the necessary data to differentiate it when the data argument is different. However, the change introduced discrepancies in the ID system and potential logic pitfalls as the processed mapping entry is at times evaluated using the "U" secondary ID and at others using the original ID system. We advise only a single ID system to be adopted across the whole contract system.

FED-02M: Desynchronization of State

TypeSeverityLocation
Logical FaultMediumFederation.sol:L211-L220

Description:

The setRevokeTransactionAndVote function will not properly nullify the votes of members that have been excluded from the Federation and will be added at a later date.

Example:

sovryn-token-bridge/bridge/contracts/Federation.sol
211function setRevokeTransactionAndVote(bytes32 _revokeTransactionID) external onlyOwner {
212    require(_revokeTransactionID != NULL_HASH, "Federation: _revokeTransactionID cannot be NULL");
213    require(processed[_revokeTransactionID] == true, "Federation: cannot revoke unprocessed TX");   
214    processed[_revokeTransactionID] = false;
215    for (uint i = 0; i < members.length; i++) {
216        votes[_revokeTransactionID][members[i]] = false;
217    }
218    emit RevokeTxAndVote(_revokeTransactionID);
219

220}

Recommendation:

We advise the system to be adjusted to account for this fact and properly account for members no longer part of the Federation. Alternatively, a new transaction ID generation system can be devised to ensure a colission will never occur thus rendering the entire function redundant.

Alleviation:

The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase.

FED-03M: Single Point of Failure

TypeSeverityLocation
Logical FaultMediumFederation.sol:L170-L197

Description:

The Federation is meant to be a decentralized entity that validates the bridge transfers, however, it suffers from a single point of failure whereby the owner has total control over who is a member of the consortium.

Example:

sovryn-token-bridge/bridge/contracts/Federation.sol
170function addMember(address _newMember) external onlyOwner
171{
172    require(_newMember != NULL_ADDRESS, "Federation: Empty member");
173    require(!isMember[_newMember], "Federation: Member already exists");
174    require(members.length < MAX_MEMBER_COUNT, "Federation: Max members reached");
175

176    isMember[_newMember] = true;
177    members.push(_newMember);
178    emit MemberAddition(_newMember);
179}
180

181function removeMember(address _oldMember) external onlyOwner
182{
183    require(_oldMember != NULL_ADDRESS, "Federation: Empty member");
184    require(isMember[_oldMember], "Federation: Member doesn't exists");
185    require(members.length > 1, "Federation: Can't remove all the members");
186    require(members.length - 1 >= required, "Federation: Can't have less than required members");
187

188    isMember[_oldMember] = false;
189    for (uint i = 0; i < members.length - 1; i++) {
190        if (members[i] == _oldMember) {
191            members[i] = members[members.length - 1];
192            break;
193        }
194    }
195    members.length -= 1;
196    emit MemberRemoval(_oldMember);
197}

Recommendation:

We advise this trait to be removed from the system and instead the members of the federation themselves to vote on new inclusions or removals to and from the Federation.

Alleviation:

This functionality is meant to be deprecated in the future and as such the exhibit can be considered null.

FED-04M: Inconsistent Input Sanitization

Description:

According to changeRequirement, the minimum value of required can only be 2 meaning that the removeMember function should validate that members.length > 2 prior to the removal rather than members.length > 1.

Example:

sovryn-token-bridge/bridge/contracts/Federation.sol
181function removeMember(address _oldMember) external onlyOwner
182{
183    require(_oldMember != NULL_ADDRESS, "Federation: Empty member");
184    require(isMember[_oldMember], "Federation: Member doesn't exists");
185    require(members.length > 1, "Federation: Can't remove all the members");
186    require(members.length - 1 >= required, "Federation: Can't have less than required members");
187

188    isMember[_oldMember] = false;
189    for (uint i = 0; i < members.length - 1; i++) {
190        if (members[i] == _oldMember) {
191            members[i] = members[members.length - 1];
192            break;
193        }
194    }
195    members.length -= 1;
196    emit MemberRemoval(_oldMember);
197}
198

199function getMembers() external view returns (address[] memory)
200{
201    return members;
202}
203

204function changeRequirement(uint _required) external onlyOwner validRequirement(members.length, _required)
205{
206    require(_required >= 2, "Federation: Requires at least 2");
207    required = _required;
208    emit RequirementChange(_required);
209}

Recommendation:

We advise this to be done so to avoid confusion on the restrictions imposed by the contract.

Alleviation:

The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase.

FED-05M: Misleading Validation of required

TypeSeverityLocation
Logical FaultMinorFederation.sol:L113

Description:

The required member is meant to represent the minimum number of votes a transaction should have to be accepted, however, the latter conditional of the linked line will supercede it if it's relatively low.

Example:

sovryn-token-bridge/bridge/contracts/Federation.sol
113if (transactionCount >= required && transactionCount >= members.length / 2 + 1) {

Recommendation:

We advise the latter conditional to be omitted from the codebase and instead, a restriction on the values of required to be imposed ensuring that it is at least equal to or greater than members.length / 2 + 1.

Alleviation:

The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase.

Prev

Bridge.sol (BRI-M)

Next

MultiSigWallet.sol (MSW-M)

On this page