Liquid Collective - Alluvial

The reportBeacon function reports the beacon data to the protocol to correctly validate price of underlying asset. This function also makes an internal call to the _pushToRiver function. The contract tries to calculate the ETH balance after and before during the _pushToRiver call. However, it uses totalSupply instead of the current balance, which is the underlying balance.

Using total supply instead of underlying balance will lead to confusion about asset prices. The price of the asset may differ negatively, as the oracle will send less or more than the expected price.

Code Location

Oracle.1.sol

function _pushToRiver(
    uint256 _epochId,
    uint128 _balanceSum,
    uint32 _validatorCount,
    BeaconSpec.BeaconSpecStruct memory _beaconSpec
) internal {
    _clearReporting(_epochId + _beaconSpec.epochsPerFrame);

    IRiverOracleInput riverAddress = IRiverOracleInput(RiverAddress.get());
    uint256 prevTotalEth = riverAddress.totalSupply();
    riverAddress.setBeaconData(_validatorCount, _balanceSum, bytes32(_epochId));
    uint256 postTotalEth = riverAddress.totalSupply();

    uint256 timeElapsed = (_epochId - LastEpochId.get()) * _beaconSpec.slotsPerEpoch * _beaconSpec.secondsPerSlot;

    _sanityChecks(postTotalEth, prevTotalEth, timeElapsed);
    LastEpochId.set(_epochId);

    emit PostTotalShares(postTotalEth, prevTotalEth, timeElapsed, riverAddress.totalShares());
}

SOLVED: This issue was solved in the following commit:

Commit ID: 4b4ef76c93e215ceb1218d68f73a833c766fa134

During the audit, it was determined that there is a "Division by Zero" result in the _onEarnings function. In Solidity, when you try to set any value to zero, the execution will be reverted with an error message. Therefore, the transaction will also be reverted.

The sharesToMint variable tries to divide (_amount * _totalSupply() * globalFee) to ((_assetBalance() * BASE) - (_amount * globalFee)).

If (_assetBalance() * BASE) equals _amount * globalFee, the denominator will be zero. As a result, division by zero will occur.

Code Location

River.1.sol

function _onEarnings(uint256 _amount) internal override {
    uint256 globalFee = GlobalFee.get();
    uint256 sharesToMint = (_amount * _totalSupply() * globalFee) /
        ((_assetBalance() * BASE) - (_amount * globalFee));

    uint256 operatorRewards = (sharesToMint * OperatorRewardsShare.get()) / BASE;

    uint256 mintedRewards = _rewardOperators(operatorRewards);

    _mintRawShares(TreasuryAddress.get(), sharesToMint - mintedRewards);
}

SOLVED: This issue was solved by adding another denominator check to the contract.

Commit ID: 799c72d45441d6a3a0828a381149a96611ea656e

Single-step ownership change for contracts is risky. Owner addresses in River contracts can be changed in one step due to pattern in LibOwnable. If the owner's address is set to the wrong address, this could lead to funds being lost or locked.

When changing privileged roles, a two-step approach is recommended:

1. The current privileged role proposes a new address for change
2. The proposed new address then claims the privileged role in a separate transaction.

Code Location

River.1.sol

function setAdministrator(address _newAdmin) external onlyAdmin {
    LibOwnable._setAdmin(_newAdmin);
}

SOLVED: This issue was solved by implementing the two-step ownership change in the contract.

Commit ID: 2c3fdc8d2fa91c045d4d7332d81ee044b7e8f3da

Allowed users can deposit or donate ETH to the River contract. If these users accidentally send ETH to this contract, there is no way to revert this error. River's contract has no withdraw function. Therefore, users will not be able to retrieve their accidentally sent ETH.

RISK ACCEPTED: The risk of this finding was accepted.

Currently, this is intended behavior. The withdrawal process will be defined and implemented in details once the spec is written in stone. As we advance with unknowns, we currently have a stub withdrawal contract ready to accept all the exited funds, where the implementation will be changed to manage all this process.

To protect against cross-function re-entrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the withdrawal function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called nonReentrant that guards the function with a mutex against re-entrancy attacks.

Code Location

WLSETH.1.sol

function mint(address _recipient, uint256 _value) external {
    BalanceOf.set(_recipient, BalanceOf.get(_recipient) + _value);
    IRiverToken(RiverAddress.get()).transferFrom(msg.sender, address(this), _value);
}

WLSETH.1.sol

function burn(address _recipient, uint256 _value) external {
    uint256 callerUnderlyingBalance = IRiverToken(RiverAddress.get()).underlyingBalanceFromShares(
        BalanceOf.get(msg.sender)
    );
    if (_value > callerUnderlyingBalance) {
        revert BalanceTooLow();
    }
    uint256 sharesAmount = IRiverToken(RiverAddress.get()).sharesFromUnderlyingBalance(_value);
    BalanceOf.set(msg.sender, BalanceOf.get(msg.sender) - sharesAmount);
    IRiverToken(RiverAddress.get()).transfer(_recipient, sharesAmount);
}

TransferManager.1.sol

function deposit() external payable {
    _deposit(msg.sender);
}

TransferManager.1.sol

function donate() external payable {
    if (msg.value == 0) {
        revert EmptyDonation();
    }

    _onDonation(msg.value);

    emit Donation(msg.sender, msg.value);
}

SOLVED: This finding was solved for the burn() and mint() methods after implementing nonReentrant modifier for these methods.

Commit ID: e9be5d57d67568b6e47ee0748945a4884c939a4d

The transferFrom and transfer functions are declared to return a boolean return variable after successful transfers. However, it does not return any variables during calls to the WLSETH.mint() and WLSETH.burn() functions. It is important to validate these return variables. In this case, calling these functions can break any integrations or composability.

Code Location

WLSETH.1.sol

function mint(address _recipient, uint256 _value) external {
    BalanceOf.set(_recipient, BalanceOf.get(_recipient) + _value);
    IRiverToken(RiverAddress.get()).transferFrom(msg.sender, address(this), _value);
}

WLSETH.1.sol

function burn(address _recipient, uint256 _value) external {
    uint256 callerUnderlyingBalance = IRiverToken(RiverAddress.get()).underlyingBalanceFromShares(
        BalanceOf.get(msg.sender)
    );
    if (_value > callerUnderlyingBalance) {
        revert BalanceTooLow();
    }
    uint256 sharesAmount = IRiverToken(RiverAddress.get()).sharesFromUnderlyingBalance(_value);
    BalanceOf.set(msg.sender, BalanceOf.get(msg.sender) - sharesAmount);
    IRiverToken(RiverAddress.get()).transfer(_recipient, sharesAmount);
}

SOLVED: This vulnerability was resolved by Alluvial team after adding additional code that checks the return value from transferFrom() method.

Commit ID: cab51608d19a44e0c165715bc6e7a970d657b19a

River Contracts have address fields in multiple functions. These functions are missing address validations. Each address should be validated and checked to be non-zero. This is also considered a best practice.

During testing, it has been found that some of these inputs are not protected against using address(0) as the destination address.

Code Location

Functions with missing zero address checks

OracleManagerV1.initOracleManagerV1::_oracle
OperatorsManagerV1.addOperator::_operator,_feeRecipient
DepositManagerV1.initDepositManagerV1::_depositContractAddress
WLSETHV1.mint::_recipient
WLSETHV1.burn::_recipient
RiverV1.setAdministrator::_newAdmin
RiverV1.setTreasury::_newTreasury
RiverV1.setAllowlist::_newAllowlist
Firewall.constructor::governor_,executor_
Firewall.changeGovernor::newGovernor
Firewall.changeExecutor::newExecutor
ELFeeRecipientV1.initELFeeRecipientV1::_riverAddress

SOLVED: The issue was solved by adding zero address checks.

Commit ID: 4a526b0a9f82537bdf582fee1475171433149216

The ifAdmin modifier is designed for functions that can be executable by only the admin role. The isPaused function on the TUPProxy contract checks that if the current contract is paused or not. It returns true or false accordingly. However, this function has unnecessary ifAdmin modifier. Therefore, only the contract admin can view the paused state of contract.

Code Location

TUPProxy.sol

function isPaused() external ifAdmin returns (bool) {
    return StorageSlot.getBooleanSlot(_PAUSE_SLOT).value;
}

NOT APPLICABLE: This finding is not applicable because it was designed as an intended behavior.

This was intended as the ifAdmin has two roles:

• Protects sensitive calls from being performed by random actors
• Prevent method collision between proxy and implementation

The isPaused case falls in the second case as it prevents any collision with the implementation (we can have an unrelated isPaused method in the implementation and users won’t call the one in the Proxy, only the admin will)

In Solidity (pragma 0.8.0 and later), adding unchecked keyword for arithmetical operations can decrease gas usage on contracts where underflow/underflow is unrealistic. It is possible to save gas by using this keyword on multiple code locations.

Code Location

Oracle.1.sol

function isMember(address _memberAddress) external view returns (bool) {
    address[] memory members = OracleMembers.get();
    for (uint256 idx = 0; idx < members.length; ++idx) {
        if (members[idx] == _memberAddress) {
            return true;
        }
    }
    return false;
}

River.1.sol

for (uint256 idx = 0; idx < operators.length; ++idx) {
    uint256 operatorActiveValidatorCount = operators[idx].funded - operators[idx].stopped;
    totalActiveValidators += operatorActiveValidatorCount;
    validatorCounts[idx] = operatorActiveValidatorCount;
}

River.1.sol

for (uint256 idx = 0; idx < validatorCounts.length; ++idx) {
    _mintRawShares(operators[idx].feeRecipient, validatorCounts[idx] * rewardsPerActiveValidator);
}

DepositManager.1.sol

for (uint256 idx = 0; idx < receivedPublicKeyCount; idx += 1) {
    _depositValidator(publicKeys[idx], signatures[idx], withdrawalCredentials);
}

OperatorsManager.1.sol

for (uint256 idx = 0; idx < _keyCount; ++idx) {
    bytes memory publicKey = BytesLib.slice(
        _publicKeys,
        idx * ValidatorKeys.PUBLIC_KEY_LENGTH,
        ValidatorKeys.PUBLIC_KEY_LENGTH
    );

OperatorsManager.1.sol

for (uint256 idx = 0; idx < _indexes.length; ++idx) {
    uint256 keyIndex = _indexes[idx];
    ...

OperatorsManager.1.sol

for (uint256 idx = 0; idx < arr1.length; ++idx) {
    res[idx] = arr1[idx];
}
for (uint256 idx = 0; idx < arr2.length; ++idx) {
    res[idx + arr1.length] = arr2[idx];
}

OperatorsManager.1.sol

for (uint256 idx = 1; idx < operators.length; ++idx) {
    if (
        operators[idx].funded - operators[idx].stopped <
        operators[selectedOperatorIndex].funded - operators[selectedOperatorIndex].stopped
    ) {
        selectedOperatorIndex = idx;
    }
}

SOLVED: This finding was solved by applying the suggestion above for all for loops in the protocol.

Commit ID: 092c468a112955aebd8a5ed2d80507393f9836c8

In all for loops, the index variable is incremented using +=. It is known that, in loops, using ++i costs less gas per iteration than +=. This also affects incremented variables within the loop code block.

Code Location

DepositManager.1.sol

for (uint256 idx = 0; idx < receivedPublicKeyCount; idx += 1) {
    _depositValidator(publicKeys[idx], signatures[idx], withdrawalCredentials);
}

SOLVED: This finding was solved by applying the suggestion above for all for loops in the protocol.

Commit ID: 092c468a112955aebd8a5ed2d80507393f9836c8

The _depositValidator function on the DepositManagerV1 contract tries to calculate depositAmount variable correctly by using assert. It tries to divide the DEPOSIT_SIZE variable into 1000000000 wei. Subsequently, it tries to multiply the depositAmount variable with 1000000000 wei to check this calculation if equals to value variable, which is declared as DEPOSIT_SIZE already.

The assert usage here does not benefit anything to the contract, since value is hardcoded as DEPOSIT_SIZE. The result will always be the same.

Code Location

DepositManager.1.sol

uint256 value = DEPOSIT_SIZE;

uint256 depositAmount = value / 1000000000 wei;
assert(depositAmount * 1000000000 wei == value);

SOLVED: This finding was solved by removing the unnecessary assert block.

Commit ID: 4edda32d138d85e28ceb51e1519947078177a526

Some if conditions on the contract using unnecessary boolean comparisons. Removing these boolean comparisons can optimize gas usage during the deployment of contract.

Code Location

OperatorsManager.1.sol

modifier active(uint256 _index) {
    if (Operators.getByIndex(_index).active == false) {
        revert InactiveOperator(_index);
    }
    _;
}

OperatorsManager.1.sol

modifier operatorFeeRecipientOrAdmin(uint256 _index) {
    if (msg.sender == LibOwnable._getAdmin()) {
        _;
        return;
    }
    Operators.Operator storage operator = Operators.getByIndex(_index);
    if (operator.active == false) {
        revert InactiveOperator(_index);
    }
    if (msg.sender != operator.feeRecipient) {
        revert Errors.Unauthorized(msg.sender);
    }
    _;
}

OperatorsManager.1.sol

modifier operatorOrAdmin(uint256 _index) {
    if (msg.sender == LibOwnable._getAdmin()) {
        _;
        return;
    }
    Operators.Operator storage operator = Operators.getByIndex(_index);
    if (operator.active == false) {
        revert InactiveOperator(_index);
    }
    if (msg.sender != operator.operator) {
        revert Errors.Unauthorized(msg.sender);
    }
    _;
}

OperatorsManager.1.sol

function addOperator(
    string calldata _name,
    address _operator,
    address _feeRecipient
) external onlyAdmin {
    if (Operators.exists(_name) == true) {
        revert OperatorAlreadyExists(_name);
    }

SOLVED: The issue was fixed by removing unnecessary boolean comparisons from their if conditions.

Commit ID: 4e946302e7485c663613b7067c12397dc6dfc357

The _onDeposit internal function on the RiverV1 contract uses TRANSFER_MASK and DEPOSIT_MASK variables to validate if user have permission to deposit ETH to the contract or transfer ETH to allowed addresses.

The DEPOSIT_MASK and TRANSFER_MASK variables are equal to 0x1 and 0x0 in order. The first onlyAllowed check tries to validate if the _depositor have permission for both deposit and transfer operations.

River.1.sol

(AllowlistAddress.get()).onlyAllowed(_depositor, DEPOSIT_MASK + TRANSFER_MASK);

However, addition of both these masks always returns 0x1 which is equals to DEPOSIT_MASK. As a result, use of this mathematical operation is unnecessary.

Code Location

River.1.sol

function _onDeposit(
    address _depositor,
    address _recipient,
    uint256 _amount
) internal override {
    SharesManagerV1._mintShares(_depositor, _amount);
    if (_depositor == _recipient) {
        (AllowlistAddress.get()).onlyAllowed(_depositor, DEPOSIT_MASK); // this call reverts if unauthorized or denied
    } else {
        (AllowlistAddress.get()).onlyAllowed(_depositor, DEPOSIT_MASK + TRANSFER_MASK); // this call reverts if unauthorized or denied
        (AllowlistAddress.get()).onlyAllowed(_recipient, TRANSFER_MASK);
        _transfer(_depositor, _recipient, _amount);
    }
}

ACKNOWLEDGED: This finding was acknowledged.

This is intended as it shows both rights are required to perform the action, but the fact that the mask is 0 means that it currently requires no special rights. This is a value that might be changed in the future depending on the protocol’s evolution and future needs.