DAO controls critical functions, risking fund loss via role misuse.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L420-L424 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L120-L158 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/proxy/AMBeacon.sol#L63-L65

Vulnerability details

Impact

The protocol gives the DAO full control over critical operations like setting fees, reward proxies, etc via the AccessManager contract.

For example, the setRewardsProxy function in PrincipalToken.sol allows the DAO to set the rewards proxy: PrincipalToken.sol#setRewardsProxy

function setRewardsProxy(address _rewardsProxy) external restricted {
    // Note: address zero is allowed in order to disable the claim proxy
    emit RewardsProxyChange(rewardsProxy, _rewardsProxy);
    rewardsProxy = _rewardsProxy;
}

The restricted modifier checks that the caller has the REWARDS_PROXY_SETTER_ROLE (roleId 6). This is initially set to the DAO in the initializer: PrincipalToken.sol#initialize

function initialize(
    address _ibt,
    uint256 _duration,
    address _initialAuthority
) external initializer {

    // ...

    __AccessManaged_init(_initialAuthority);

  }

If a malicious actor was able to gain the REWARDS_PROXY_SETTER_ROLE they could set the rewards proxy to a malicious contract that steals funds via delegatecall.

Similarly, they could set malicious fees via the FEE_SETTER_ROLE or upgrade the protocol via the UPGRADER_ROLE.

This over-reliance on the DAO as a trusted entity could lead to a catastrophic loss of funds if the DAO multisig or individual DAO members were compromised.

Trusting the DAO with unlimited authority rather than taking a least authority approach.

The UPGRADER_ROLE is used to control who can upgrade the PrincipalToken and YieldToken implementations via the proxy beacon pattern.

For example, upgradeTo in AMBeacon.sol#upgradeTo

function upgradeTo(address newImplementation) public virtual restricted {
    _setImplementation(newImplementation);
}

Again, the restricted modifier checks for the UPGRADER_ROLE.

If this role is given too broadly, a malicious actor could upgrade critical contracts like the PrincipalToken to a malicious version that steals funds.

The root cause is not restricting the upgrade role tightly enough.

Proof of Concept

A malicious actor who gains the REWARDS_PROXY_SETTER_ROLE could set the rewardsProxy address to a malicious contract that appears benign but exploits the delegatecall in claimRewards to drain funds from PrincipalToken.sol#claimRewards

function claimRewards(bytes memory _data) external restricted {
    if (rewardsProxy == address(0)) {
        revert NoRewardsProxySet();
    }
    _data = abi.encodeWithSelector(IRewardsProxy(rewardsProxy).claimRewards.selector, _data);
    (bool success, ) = rewardsProxy.delegatecall(_data);
    if (!success) {
        revert ClaimRewardsFailed();
    }
}

By encoding malicious logic in _data, they could extract funds via the delegatecall.

For example, the malicious proxy could implement a claimRewards function that calls IERC20(principalTokenAddress).transfer(attackerAddress, fundsToSteal).

This would allow the attacker to immediately drain a significant portion of funds from the protocol by invoking claimRewards once.

Similarly, the attacker could set max fees like setTokenizationFee(100000) by gaining the FEE_SETTER_ROLE which would allow draining funds more slowly over time.

Gaining the UPGRADER_ROLE would allow pushing a malicious upgrade to PrincipalToken that could steal funds in arbitrary ways.

Exploiting the Upgradeability Issue

The attacker first needs to gain access to the UPGRADER_ROLE, likely by compromising the DAO multisig or an authorized signer.

Once they have the upgrade role, they can create a malicious PrincipalToken implementation and upgrade the proxy to it by calling upgradeTo in AMBeacon.sol:

function upgradeTo(address newImplementation) public virtual restricted {
    _setImplementation(newImplementation);
}

The upgraded PrincipalToken could have an evil redeem function that transfers funds to the attacker before calling the original logic:

function redeem(...) {
    IERC20(asset).transfer(attacker, fundsToSteal); 
    originalRedeemLogic();
}

This would allow them to immediately drain funds from the protocol by redeeming any amount.

Tools Used

Manual review

Recommended Mitigation Steps

The DAO roles should be segmented so no single entity has control over all operations. Upgradeability should also be limited to a minimal set of trusted accounts. An on-chain admin like Defender could help enforce tighter access controls.

Assessed type

Access Control

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #26

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid