Description

The vesting admin has the ability to call the following two functions:

function vestingDeposit(
    address from,
    uint256 amount
) public nonReentrant onlyRole(VESTING_ADMIN_ROLE) returns (uint256) {
!    SafeERC20.safeTransferFrom(IERC20(asset()), from, address(this), amount);
    _vestingDeposit += amount;
    emit VestingDeposit(from, amount);

    return amount;
}

function vestingWithdraw(
    address to,
    uint256 amount
) public nonReentrant onlyRole(VESTING_ADMIN_ROLE) returns (uint256) {
    if (amount > getVestingDeposit()) {
        revert VestingWithdrawAmountExceeded(amount, getVestingDeposit());
    }
!    SafeERC20.safeTransfer(IERC20(asset()), to, amount);
    _vestingDeposit -= amount;
    emit VestingDeposit(to, amount);

    return amount;
}

By calling vestingDeposit(victim, amount); immediately followed by vestingWithdraw(attacker, amount);, the vesting admin essentially has the following primitive, where the victim, attacker, and amount are arbitrarily controllable values:

asset().transferFrom(victim, attacker, amount);`

Impact

Depending on which asset the contract is configured to use, the vesting admin may be able to drain the entire vault by using the aforementioned transferFrom primitive, where the victim is the vault address. In the latest OpenZeppelin implementation of the ERC20 contract, transferFrom would require an approval from the vault address to the vault address (i.e., there is a requirement that allowance(vault, vault) > 0 — see source here↗).

However, the SoleraStaking contract is ERC-20 implementation-agnostic. Other implementations may not have this same requirement; for example, WETH and DAI both exclude this check (see source here↗).

Regardless of the ERC-20 that SoleraStaking is intended to be deployed with, leaving the vesting admin with this ability hinders the reusability of this contract. In the future, other developers who want to build on the contract may not be aware of the requirement that prevents a critical bug from existing.

And in any case, if a user leaves open approvals to the SoleraStaking contract (e.g., if they approve more than necessary to call deposit, which relies on allowances), or if the user approves and deposits in separate transactions, then the vesting admin can steal the user's approved assets.

Exploitation in any scenario requires that the vesting admin is malicious or compromised, so we rate this finding as Low likelihood. However, in our opinion, leaving the vesting admin with this primitive entirely defeats the purpose of having a separate role — this finding is therefore part of the contract's threat model.

Recommendations

The vesting admin should not have the ability to withdraw from an arbitrary address. Though it may cost the vesting admin slightly more gas, we recommend having the vesting admin transfer the assets to their wallet first. The vestingDeposit function should be changed to only allow deposits from the vesting admin's wallet.

Remediation

This issue has been acknowledged by Solera Markets, and a fix was implemented in PR #11↗.