code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/vToken.sol#L85

Vulnerability details

Impact

The ORDERER_ROLE role has the ability to arbitrarily transfer user funds, and this role is shared between both the orderer and people who can rebalance the index.

Even if the owner is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation. See this example where a similar finding has been flagged as a high-severity issue. I've downgraded this instance to be a medium since it requires a malicious manager.

Proof of Concept

The role is given to the orderer so it has the ability to add/remove funds during Uniswap operations: File: contracts/vToken.sol (lines 80-87)

    /// @inheritdoc IvToken
    function transferFrom(
        address _from,
        address _to,
        uint _shares
    ) external override nonReentrant onlyRole(ORDERER_ROLE) {
        _transfer(_from, _to, _shares);
    }

The role is also required to initiate rebalances: File: contracts/TopNMarketCapIndex.sol (lines 67-68)

    /// @notice Reweighs index assets according to the latest market cap data for specified category
    function reweight() external override onlyRole(ORDERER_ROLE) {

File: contracts/TrackedIndex.sol (lines 56-57)

    /// @notice Reweighs index assets according to the latest market cap data
    function reweight() external override onlyRole(ORDERER_ROLE) {

It is not necessary for the person/tool initiating reweights to also have the ability to arbitrarily transfer funds, so they should be separate roles. If the orderer also needs to be able to reweight, the orderer should also be given the new role.

Tools Used

Code inspection

Recommended Mitigation Steps

Split the role into two, and only give the ORDERER_ROLE role to the orderer

jn-lp commented 2 years ago

ORDERER_ROLE role is only given to Orderer contract by multisig, which must have the ability to reweight indices as well as to transferFrom on vToken contract

moose-code commented 2 years ago

I agree with the warden at the very least there is only benefit in splitting this role out appropriately into two roles. There is likely a case where the ordered and index rebalancers aren't the same.

code-423n4 / 2022-04-phuture-findings