Fractional v2 contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L131-L132

Vulnerability details

OwnerChanged() protection in Vault.sol can be bypassed

Impact

In case of compromised _target contract, a malicious user can bypass the OwnerChanged() check and do anything. Potentially drain the whole vault.

Proof of Concept

The purpose of the OwnerChanged() check is to protect against delegatecall from malicious contract.

src/Vault.sol:131-132:

        (success, response) = _target.delegatecall{gas: stipend}(_data);
        if (owner_ != owner) revert OwnerChanged(owner_, owner);

However, the easiest way to bypass the check is to change owner back to the original one after draining the vault. The steps would be:

1. Change the owner to the manipulated contract address.
2. Drain the vault.
3. Change the owner back to owner_.

Since the owner is changed back, the following check won't be triggered.

if (owner_ != owner) revert OwnerChanged(owner_, owner);

Tools Used

Mannual analysis.

Recommended Mitigation Steps

• Distribute the authority to several previlege roles, mitigate the impact of one single owner.
• Warn users at the beginning about the potential exploit due to untrusted plugins.
• Maybe set some timelock to delay the transfer in some cases.

EVENT IS MISSING INDEXED FIELDS

Each event should use three indexed fields if there are three or more fields.

src/FERC1155.sol
134:        emit SingleApproval(_owner, _operator, _id, _approved);
179:        emit ApprovalForAll(_owner, _operator, _approved);
193:        emit SingleApproval(msg.sender, _operator, _id, _approved);
224:        emit SetRoyalty(_receiver, _id, _percentage);
273:        emit TransferSingle(msg.sender, _from, _to, _id, _amount);

src/VaultFactory.sol
81-88:
        emit DeployVault(
            tx.origin,
            msg.sender,
            _owner,
            seed,
            salt,
            address(vault)
        );

src/VaultRegistry.sol
176:        emit VaultDeployed(vault, _token, nextId[_token]);


src/modules/Buyout.sol
100-106:
        emit Start(
            _vault,
            msg.sender,
            block.timestamp,
            buyoutPrice,
            fractionPrice
        );
223:        emit End(_vault, State.SUCCESS, proposer);
237:        emit End(_vault, State.INACTIVE, proposer);
272:        emit Cash(_vault, msg.sender, buyoutShare);

src/modules/Migration.sol
243-250:
        emit VaultMigrated(
            _vault,
            newVault,
            _proposalId,
            proposal.modules,
            proposal.plugins,
            proposal.selectors
        );

281-286:
        emit FractionsMigrated(
            _vault,
            proposal.newVault,
            _proposalId,
            proposal.newFractionSupply
        );

0 amount check for buyout

src/modules/Buyout.sol:73-82:

        uint256 depositAmount = IERC1155(token).balanceOf(msg.sender, id);

        // Transfers fractional tokens into the buyout pool
        IERC1155(token).safeTransferFrom(
            msg.sender,
            address(this),
            id,
            depositAmount,
            ""
        );

Before transfer, can check if the depositAmount is > 0.

buyoutInfo[_vault] can be deleted if all cash were settled

src/Vault.sol:

    function cash(address _vault, bytes32[] calldata _burnProof) external {
        // Reverts if address is not a registered vault
        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
            _vault
        );
        if (id == 0) revert NotVault(_vault);
        // Reverts if auction state is not successful
        (, , State current, , uint256 ethBalance, ) = this.buyoutInfo(_vault);
        State required = State.SUCCESS;
        if (current != required) revert InvalidState(required, current);
        // Reverts if caller has a balance of zero fractional tokens
        uint256 tokenBalance = IERC1155(token).balanceOf(msg.sender, id);
        if (tokenBalance == 0) revert NoFractions();

        // Initializes vault transaction
        bytes memory data = abi.encodeCall(
            ISupply.burn,
            (msg.sender, tokenBalance)
        );
        // Executes burn of fractional tokens from caller
        IVault(payable(_vault)).execute(supply, data, _burnProof);

        // Transfers buyout share amount to caller based on total supply
        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
        uint256 buyoutShare = (tokenBalance * ethBalance) /
            (totalSupply + tokenBalance);
        _sendEthOrWeth(msg.sender, buyoutShare);
        // Emits event for cashing out of buyout pool
        emit Cash(_vault, msg.sender, buyoutShare);
    }

After all funds are settled, buyoutInfo[_vault] can be deleted.

Unused return value

src/Vault.sol

67:
    function execute() {
        // ...
        (success, response) = _execute(_target, _data);
    }

The return value is garanteed to be true due to the following:

131-138:
    function _execute() {
        // ...
        (success, response) = _target.delegatecall{gas: stipend}(_data);
        if (owner_ != owner) revert OwnerChanged(owner_, owner);

        // Revert if execution was unsuccessful
        if (!success) {
            if (response.length == 0) revert ExecutionReverted();
            _revertedWithReason(response);
        }
    }

Suggestion: Remove the unnecessary return values.

Why use array when there is only 1 element

src/modules/Minter.sol

    function getPermissions()
        public
        view
        returns (Permission[] memory permissions)
    {
        permissions = new Permission[](1);
        permissions[0] = Permission(
            address(this),
            supply,
            ISupply.mint.selector
        );
    }

Suggestion: Just use the struct rather than array.

Comment typo "burn" -> "mint"

It should be "// If the burn reverted"

src/targets/Supply.sol:136:

            // If the mint reverted

Comment typo "for" -> "with"

src/modules/Buyout.sol
146:
    /// @notice Buys fractional tokens in exchange for ether from a pool

Should be

    /// @notice Buys fractional tokens in exchange with ether from a pool

Suggestion: Change the "for" to "with".

FOR-LOOPS GAS-SAVING

There are several for loops can be improved to save gas

• NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

src/modules/protoforms/BaseVault.sol
64:        for (uint256 i = 0; i < _tokens.length; ) {
83:        for (uint256 i = 0; i < _tokens.length; ) {
107:       for (uint256 i = 0; i < _tokens.length; ++i) {


src/utils/MerkleBase.sol
51:        for (uint256 i = 0; i < _proof.length; ++i) {

• ++I COSTS LESS GAS COMPARED TO I++ OR I += 1

src/Vault.sol
77-78:
        uint256 length = _selectors.length;
        for (uint256 i = 0; i < length; i++) {

103-104:
        uint256 length = _selectors.length;
        for (uint256 i = 0; i < length; i++) {

suggestion: using ++i instead of i++ to increment the value of an uint variable. And it can be unchecked to save gas.

Just like here: src/modules/Buyout.sol

        for (uint256 i; i < permissions.length; ) {
            // Hashes permission into leaf node
            nodes[i] = keccak256(abi.encode(permissions[i]));
            // Can't overflow since loop is a fixed size
            unchecked {
                ++i;
            }

Guaranteed non-overflow can be unchecked

src/utils/MerkleBase.sol
147:
                for (uint256 i; i < length - 1; i += 2) {

USE CALLDATA INSTEAD OF MEMORY

When arguments are read-only on external functions, the data location can be calldata:

src/FERC1155.sol:68-72:

    function emitSetURI(uint256 _id, string memory _uri) external {
        if (msg.sender != metadata[_id])
            revert InvalidSender(metadata[_id], msg.sender);
        emit URI(_uri, _id);
    }

src/Vault.sol
73:     function install(bytes4[] memory _selectors, address[] memory _plugins)
101:    function uninstall(bytes4[] memory _selectors) external {


src/VaultRegistry.sol
51-55:
    function create(
        bytes32 _merkleRoot,
        address[] memory _plugins,
        bytes4[] memory _selectors
    ) external returns (address vault) {

67-72:
    function createFor(
        bytes32 _merkleRoot,
        address _owner,
        address[] memory _plugins,
        bytes4[] memory _selectors
    ) external returns (address vault) {

83-87:
    function createCollection(
        bytes32 _merkleRoot,
        address[] memory _plugins,
        bytes4[] memory _selectors
    ) external returns (address vault, address token) {

102-107:
    function createInCollection(
        bytes32 _merkleRoot,
        address _token,
        address[] memory _plugins,
        bytes4[] memory _selectors
    ) external returns (address vault) {

147-152:
    function createCollectionFor(
        bytes32 _merkleRoot,
        address _controller,
        address[] memory _plugins,
        bytes4[] memory _selectors
    ) public returns (address vault, address token) {

165-170:
    function _deployVault(
        bytes32 _merkleRoot,
        address _token,
        address[] memory _plugins,
        bytes4[] memory _selectors
    ) private returns (address vault) {

Duplicate code could be replaced with a function

src/targets/Supply.sol 25-108: 115-198:

Most of the code block is duplicate.

What really needed are the following arguments:

_to,
_value

REGISTRY_MINT_SIG_PTR,
REGISTRY_MINT_SIGNATURE,
REGISTRY_MINT_TO_PRT, 
REGISTRY_MINT_VALUE_PTR,

REGISTRY_BURN_SIG_PTR,
REGISTRY_BURN_SIGNATURE,
REGISTRY_BURN_TO_PRT, 
REGISTRY_BURN_VALUE_PTR,

Suggestion: Using a function to save gas.

X = X + Y IS CHEAPER THAN X += Y

Consider use X = X + Y or X = X - Y to save gas.

src/FERC1155.sol
86:        totalSupply[_id] += _amount;
270-271:
        balanceOf[_from][_id] -= _amount;
        balanceOf[_to][_id] += _amount;


src/modules/Buyout.sol
139:        buyoutInfo[_vault].ethBalance -= ethAmount;
176:        buyoutInfo[_vault].ethBalance += msg.value;


src/modules/Migration.sol
123-124:
        proposal.totalEth += msg.value;
        userProposalEth[_proposalId][msg.sender] += msg.value;
134-135:
        proposal.totalFractions += _amount;
        userProposalFractions[_proposalId][msg.sender] += _amount;

            for (uint256 i; i < length - 1; i += 2) {

156:        proposal.totalFractions -= amount;
160:        proposal.totalEth -= ethAmount;


src/utils/MerkleBase.sol
147:            for (uint256 i; i < length - 1; i += 2) {
190:            ceil -= pOf2;

FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

src/FERC1155.sol

56-63:
    function burn(
        address _from,
        uint256 _id,
        uint256 _amount
    ) external onlyRegistry {
        _burn(_from, _id, _amount);
        totalSupply[_id] -= _amount;
    }

79-87:
    function mint(
        address _to,
        uint256 _id,
        uint256 _amount,
        bytes memory _data
    ) external onlyRegistry {
        _mint(_to, _id, _amount, _data);
        totalSupply[_id] += _amount;
    }

198-200:
    function setContractURI(string calldata _uri) external onlyController {
        contractURI = _uri;
    }

205-211:
    function setMetadata(address _metadata, uint256 _id)
        external
        onlyController
    {
        metadata[_id] = _metadata;
        emit SetMetadata(_metadata, _id);
    }

217-225:
    function setRoyalties(
        uint256 _id,
        address _receiver,
        uint256 _percentage
    ) external onlyController {
        royaltyAddress[_id] = _receiver;
        royaltyPercent[_id] = _percentage;
        emit SetRoyalty(_receiver, _id, _percentage);
    }

229-236:
    function transferController(address _newController)
        external
        onlyController
    {
        if (_newController == address(0)) revert ZeroAddress();
        _controller = _newController;
        emit ControllerTransferred(_newController);
    }