search

sherlock-audit / 2023-12-flatmoney-judging

9 stars 7 forks source link

stacey - The initialization contract `FlatcoinVault` fails if `_owner` is not the `msg.sender` #263

Closed sherlock-admin closed 5 months ago

sherlock-admin commented 5 months ago

stacey

medium

The initialization contract FlatcoinVault fails if _owner is not the msg.sender

medium

The initialization contract FlatcoinVault fails if _owner is not the msg.sender

Summary

The following methods are called during the initialization process: setMaxFundingVelocity, setMaxVelocitySkew, setStableCollateralCap, setSkewFractionMax, setExecutabilityAge. These operations can be reverted if executed by someone other than the owner.

Vulnerability Detail

The vulnerability resides in the FlatcoinVault.initialize(...) function: [source] (https://github.com/sherlock-audit/2023-12-flatmoney/blob/bba4f077a64f43fbd565f8983388d0e985cb85db/flatcoin-v1/src/FlatcoinVault.sol#L109C1-L131C6)

The function __Ownable_init() sets msg.sender as the owner of the FlatcoinVault contract. Subsequently, the function _transferOwnership(_owner) changes the contract's ownership to _owner, which is an argument of the`initialize(...) function.

Functions setMaxFundingVelocity, setMaxVelocitySkew, setStableCollateralCap, setSkewFractionMax, setExecutabilityAge have a modifier onlyOwner. However, msg.sender for these functions is msg.sender of initialize(...) function, not the owner set in the function _transferOwnership(_owner).

Therefore, if the owner set in _transferOwnership(_owner) is not the same as msg.sender of initialize(...) function, the function will fail.

Impact

The impact of this vulnerability on smart contract operation is very large. If initialization fails, the contract will not work as intended. To solve this problem, the contract needs to be re-deployed.

Code Snippet

If the argument _owner: admin is changed to _owner: carol in Setup.sol and the function vaultProxy.initialize(...) is called, the operation will fail.


        vaultProxy.initialize({
-           _owner: admin,
+           _owner: carol,
            ...
        });

result:


Failing tests:
Encountered 1 failing test in test/unit/Flatcoin-Vault/FlatcoinVault.t.sol:FlatcoinVaultTest
[FAIL. Reason: setup failed: revert: Ownable: caller is not the owner] setUp() (gas: 0)

Tool used

Manual Review

Recommendation

To resolve this issue, call _transferOwnership(_owner) at the end of the FlatcoinVault.initialize(...) function.

    function initialize(
        address _owner,
        IERC20Upgradeable _collateral,
        uint256 _maxFundingVelocity,
        uint256 _maxVelocitySkew,
        uint256 _skewFractionMax,
        uint256 _stableCollateralCap,
        uint64 _minExecutabilityAge,
        uint64 _maxExecutabilityAge
    ) external initializer {
        if (address(_collateral) == address(0)) revert FlatcoinErrors.ZeroAddress("collateral");

        __Ownable_init();
-       _transferOwnership(_owner);

        collateral = _collateral;

        setMaxFundingVelocity(_maxFundingVelocity);
        setMaxVelocitySkew(_maxVelocitySkew);
        setStableCollateralCap(_stableCollateralCap);
        setSkewFractionMax(_skewFractionMax);
        setExecutabilityAge(_minExecutabilityAge, _maxExecutabilityAge);

+        _transferOwnership(_owner);
    }
sherlock-admin commented 5 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

nevillehuang commented 4 months ago

Low severity, even if true, core functionalities are not impacted given the original admin deploying contracts can still call admin only functions as needed.