Omniscia Sovryn Audit

Bridge Manual Review Findings

Bridge Manual Review Findings

BRI-01M: Incorrect Bytes Conversion

TypeSeverityLocation
Logical FaultMediumBridge.sol:L493-L500

Description:

The bytesToBytes32 performs a conversion from a dynamic length bytes array to a bytes32 value ignoring the length of the array.

Example:

sovryn-token-bridge/bridge/contracts/Bridge.sol
494if (source.length == 0) {
495    return 0x0;
496}
497assembly {
498    _result := mload(add(source, 32))
499}

Recommendation:

We advise the length of source to be mandated to be equal to 32 as otherwise trailing values of the source may be omitted causing an incorrect assessment.

Alleviation:

The function implementation was replaced by _isZeroValue which instead validates all members of the bytes memory array and properly accounts for cases with over 32 items.

BRI-02M: Insufficient Smart Contract Invocation

TypeSeverityLocation
Logical FaultMediumBridge.sol:L457

Description:

The isContract check imposed by receiveEthAt can be bypassed by a newly created contract, rendering the protection measure entirely null.

Example:

sovryn-token-bridge/bridge/contracts/Bridge.sol
426function receiveEthAt(address _receiver, bytes calldata _extraData) whenNotUpgrading whenNotPaused nonReentrant external payable {
427    require(msg.value > 0  && !(Address.isContract(msg.sender)) && (WETHAddr != address(0)), "Set WETHAddr. Send not from SC");
428    if (!ethFirstTransfer) {
429        ethFirstTransfer = true;
430    }
431    crossTokens(WETHAddr, _receiver, msg.value, _extraData);
432}

Recommendation:

We advise the measure to be carefully assessed with regards to its necessity and if deemed redundant to be omitted from the codebase as it currently provides a false sense of security.

Alleviation:

The measure was assessed as redundant by the Sovryn team and thus removed from the codebase safely.

BRI-03M: Unconventional Invocation Restriction

TypeSeverityLocation
Logical FaultMinorBridge.sol:L457

Description:

The withdrawAllEthFees function mandates that address(this).balance is greater than or equal to ethFeeCollected, a trait that would cause the function to throw regardless on the transfer in the ensuing line.

Example:

sovryn-token-bridge/bridge/contracts/Bridge.sol
456function withdrawAllEthFees(address payable _to) public payable onlyOwner {
457    require(address(this).balance >= ethFeeCollected);
458    uint256 sendEthFeeCollected = ethFeeCollected;
459    ethFeeCollected = 0;
460    _to.transfer(sendEthFeeCollected);
461}

Recommendation:

We advise the calculation of the minimum between address(this).balance and ethFeeCollected instead as the contract may indeed have a less balance than the fees due to mathematical truncations and more.

Alleviation:

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

BRI-04M: Misleading Error Message

TypeSeverityLocation
Standard ConformityInformationalBridge.sol:L190

Description:

The error message signals EIP-777 compliancy, however, the function that fails is onTokensMinted which is not denoted in the standard.

Example:

sovryn-token-bridge/bridge/contracts/Bridge.sol
158function _acceptCrossToSideToken(
159    address receiver,
160    address tokenAddress,
161    uint8 decimals,
162    uint256 granularity,
163    uint256 amount,
164    string memory symbol,
165    bytes memory userData
166) private {
167    (uint256 calculatedGranularity,uint256 formattedAmount) = Utils.calculateGranularityAndAmount(decimals, granularity, amount);
168    ISideToken sideToken = mappedTokens[tokenAddress];
169    if (address(sideToken) == NULL_ADDRESS) {
170        sideToken = _createSideToken(tokenAddress, symbol, calculatedGranularity);
171    } else {
172        require(calculatedGranularity == sideToken.granularity(), "Bridge: Granularity differ from side token");
173    }
174// Enable Mint to SC without ERC777 interface with user data = 0x00, by using erc777Converter
175    if(receiver.isContract() && bytesToBytes32(userData) == bytesToBytes32(abi.encodePacked(address(0)))) {
176        userData = abi.encode(receiver);
177        receiver = erc777ConverterAddr;
178    }
179

180    sideToken.mint(receiver, formattedAmount, userData, "");    
181

182    if (receiver.isContract()) {
183   (bool success, bytes memory errorData) = receiver.call(
184           abi.encodeWithSignature("onTokensMinted(uint256,address,bytes)", formattedAmount, sideToken, userData)
185       );
186    //    if (!success) {
187    //         emit ErrorTokenReceiver(errorData);
188    //     }
189// Revert on error      
190      require(success == true, "Sending to Smart Contract with userData!=0 requires ERC777 interface on receiver");
191    }
192    emit AcceptedCrossTransfer(tokenAddress, receiver, amount, decimals, granularity, formattedAmount, 18, calculatedGranularity, userData);
193}

Recommendation:

We advise the error-message to be adjusted to properly indicate the standard conformity requested.

Alleviation:

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

Prev

AllowTokens.sol (ATS-M)

Next

Federation.sol (FED-M)

On this page