0xDazai - addAsset() function missing access control , making it able to be called from end-user

addAsset() function missing access control , making it able to be called from end-user

Medium

Summary

The Stargate.sol contract serves as the Asset Module for non-staked liquidity pools within the Stargate Finance ecosystem. It encapsulates the pricing logic and maintains essential information for the liquidity provider (LP) pools.
The StakedStargateAM contract functions similarly for staked liquidity pools, storing the corresponding pricing logic and fundamental data for these staked LP pools.
Upon reviewing the NATSPEC documentation for both contracts, it is specified that direct interaction with the Staked Stargate Asset Module should be restricted to the Registry, the contract owner, or through an actionHandler. However, there is potential security concern.

Vulnerability Detail

The addAsset() function in both contracts lacks an appropriate Access Modifier, which is necessary to enforce the intended restrictions on function calls. This absence allows any end-user to invoke these functions, contrary to the documented usage policy.

While the StargateAM::addAsset() function includes validations to ensure that the poolId is a valid address and is permitted, it is crucial to enforce that only the Registry or the contract owner has the authority to determine the circumstances under which a poolId should be added to the StargateAssetModule.

Impact

Any end-user can interact with this function directly. This unrestricted access poses a significant security risk, potentially compromising the integrity of the Stargate Asset Module.

Code Snippet

https://github.com/arcadia-finance/accounts-v2/blob/9b24083cb832a41fce609a94c9146e03a77330b4/src/asset-modules/Stargate-Finance/StargateAM.sol#L62-L77

    function addAsset(uint256 poolId) external {
        address asset = address(SG_FACTORY.getPool(poolId));
        if (asset == address(0)) revert InvalidPool();

        address underlyingAsset = IPool(asset).token();
        if (!IRegistry(REGISTRY).isAllowed(underlyingAsset, 0)) revert UnderlyingAssetNotAllowed();

        inAssetModule[asset] = true;

        bytes32[] memory underlyingAssets_ = new bytes32[](1);
        underlyingAssets_[0] = _getKeyFromAsset(underlyingAsset, 0);
        assetToUnderlyingAssets[_getKeyFromAsset(asset, 0)] = underlyingAssets_;

        // Will revert in Registry if asset was already added.
        IRegistry(REGISTRY).addAsset(asset);
    }

https://github.com/arcadia-finance/accounts-v2/blob/9b24083cb832a41fce609a94c9146e03a77330b4/src/asset-modules/Stargate-Finance/StakedStargateAM.sol#L62-L71

    function addAsset(uint256 pid) external {
        // poolInfo is an array -> will revert on a non-existing pid.
        (address stargatePool,,,) = LP_STAKING_TIME.poolInfo(pid);

        if (!IRegistry(REGISTRY).isAllowed(stargatePool, 0)) revert PoolNotAllowed();
        if (assetState[stargatePool].allowed) revert AssetAlreadySet();

        assetToPid[stargatePool] = pid;
        _addAsset(stargatePool);
    }

Tool used

Manual Review

Recommendation

I recommend implementing strict Access Modifiers for the addAsset() function in both contracts to align with the intended access control and to mitigate any unauthorized interactions that could compromise the integrity of the protocol.

Duplicate of #62

sherlock-audit / 2023-12-arcadia-judging

0xDazai - 0xDazai - addAsset() function missing access control , making it able to be called from end-user #122