#5 Unauthorized Asset Withdrawal in SmartVaultV3::(removeCollateralNative, removeCollateral, removeAsset) resulting in Potential Undercollateralization

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/c12272f2eec533019f2d255ab690f6892027f112/contracts/SmartVaultV3.sol#L135C1-L140C6

https://github.com/Cyfrin/2023-12-the-standard/blob/c12272f2eec533019f2d255ab690f6892027f112/contracts/SmartVaultV3.sol#L142C1-L147C6

https://github.com/Cyfrin/2023-12-the-standard/blob/c12272f2eec533019f2d255ab690f6892027f112/contracts/SmartVaultV3.sol#L149C1-L154C6

Summary

The SmartVaultV3 contract includes functions (removeCollateralNative, removeCollateral, and removeAsset) that allow the owner to withdraw collateral and assets from the vault. If these functions are used improperly or maliciously, they could lead to the vault becoming undercollateralized, which would undermine the security and solvency of the system.

Vulnerability Details

Functions:

    function removeCollateralNative(uint256 _amount, address payable _to) external onlyOwner {
        require(canRemoveCollateral(getTokenManager().getToken(NATIVE), _amount), UNDER_COLL);
        (bool sent,) = _to.call{value: _amount}("");
        require(sent, "err-native-call");
        emit CollateralRemoved(NATIVE, _amount, _to);
    }

    function removeCollateral(bytes32 _symbol, uint256 _amount, address _to) external onlyOwner {
        ITokenManager.Token memory token = getTokenManager().getToken(_symbol);
        require(canRemoveCollateral(token, _amount), UNDER_COLL);
        IERC20(token.addr).safeTransfer(_to, _amount);
        emit CollateralRemoved(_symbol, _amount, _to);
    }

    function removeAsset(address _tokenAddr, uint256 _amount, address _to) external onlyOwner {
        ITokenManager.Token memory token = getTokenManager().getTokenIfExists(_tokenAddr);
        if (token.addr == _tokenAddr) require(canRemoveCollateral(token, _amount), UNDER_COLL);
        IERC20(_tokenAddr).safeTransfer(_to, _amount);
        emit AssetRemoved(_tokenAddr, _amount, _to);
    }

The functions in question are protected by the onlyOwner modifier, which restricts their execution to the owner of the contract. However, there is no additional governance or multi-signature control over these critical functions, which means that a single account has the power to potentially destabilize the entire vault.

Impact

This is an access control vulnerability that could lead to economic damage due to improper asset management. If the owner's account is compromised or the owner acts maliciously, they could drain the assets from the vault, leaving it unable to back the minted EUROs. This could lead to a loss of user funds and damage the credibility of the system.

Tools Used

Manual Review.

Recommendations

To mitigate this risk, the contract could implement a multi-signature requirement for these sensitive functions, or introduce a time lock that delays the withdrawal of assets, allowing for community intervention if necessary.

Example - A withdrawal request is recorded with a timestamp, and the actual withdrawal can only occur after the time lock duration has passed. This gives the community or other governance mechanisms time to react if the withdrawal is not in the best interest of the system. Additionally, events could be emitted to log these requests for transparency..