Fractional v2 contest - neumo's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L247 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L222

Vulnerability details

Impact

Function setRoyalties does not check that the value of _percentage is less or equal to 100. Function royaltyInfo in L247 could return a royaltyAmount greater than _salePrice.

Proof of Concept

This simple test shows how the controller can set a value of percentage greater than 100%:

// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.13;

import "./TestUtil.sol";

contract FERC1155Test is TestUtil {
    string tokenURI = "token uri";
    string contractURI = "contract uri";
    event URI(string value, uint256 indexed id);

    /// =================
    /// ===== SETUP =====
    /// =================
    function setUp() public {
        setUpContract();
        alice = setUpUser(111, 1);
        bob = setUpUser(222, 2);
        setUpCreateFor(alice);
        setUpMetadata(alice);

        vm.label(address(this), "FERC1155Test");
        vm.label(alice.addr, "Alice");
        vm.label(bob.addr, "Bob");
    }

    /// ============================
    /// ===== SET ROYALTIES =====
    /// ============================
    function testSetRoyalties() public {
        uint256 _id = 1;
        // We set a percentage of 150%
        uint256 _percentage = 150;
        uint256 _salePrice = 10000;
        // We expect a royalty amount greater that sale price (50% greater)
        uint256 _expectedRoyaltyAmount = 15000;
        (address receiver, uint256 royaltyAmount) = fERC1155.royaltyInfo(_id, _salePrice);
        // assert royalty amount is 0 before setting royalties
        assertEq(royaltyAmount, uint256(0));
        // set royalties for token id 1 to 150% (beneficiary of royalties being Bob)
        fERC1155.setRoyalties(_id, bob.addr, _percentage);
        (receiver, royaltyAmount) = fERC1155.royaltyInfo(_id, _salePrice);
        // assert royalty amount is the expected amount
        assertEq(royaltyAmount, uint256(_expectedRoyaltyAmount));
    }
}

Tools Used

Forge

Recommended Mitigation Steps

Consider adding a check in FERC1155.sol just before L222, in function setRoyalties, to check that _percentage <= 100. Some example implementations like this do exactly this.

• Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible:

  • Consider making transferOwnership a two step process (Vault.sol L93).
  • Consider making transferController a two step process (FERC1155.sol L229).

• Bad natspec comment @notice in FERC1155.sol L238

  • @notice states "Sets the token royalties" for function royaltyInfo which is view and sets no storage value at all. Consider changing to sommething like "Get token royalty info".

• Bad natspec comment @param in FERC1155.sol L239

  • @param states "_id Token ID royalties are being updated for" for function royaltyInfo which is view and sets no storage value at all. Consider changing to something like "_id Token ID royalty info is retrieved for".

• Missing @return natspec for values returned by functions:

  • FERC1155.sol (L241) - function royaltyInfo
  • FERC1155.sol (L291) - function uri
  • FERC1155.sol (L302) - function controller
  • FERC1155.sol (L309) - function INITIAL_CONTROLLER
  • FERC1155.sol (L314) - function VAULT_REGISTRY
  • FERC1155.sol (L324) - function _computePermitStructHash
  • FERC1155.sol (L350) - function _computePermitAllStructHash
  • BaseVault.sol (L34) - function deployVault

• Consider adding a check in FERC1155.sol just before L222, in function setRoyalties, to check that _receiver != address(0) if _percentage > 0

• Missing zero address validation: transferOwnership (L95) in Vault.sol does not check if new owner is different from address(0). Setters of address type parameters should include a zero-address check otherwise contract functionality may become inaccessible.

• In Vault.sol function install does not check that a selector is already present in the methods mapping. This could cause that, after calling install, some of the selectors of a preexisting plugin point to another plugin (the newly installed) while others still point to the correct plugin. This could lead to confusion. Consider reverting if trying to install a selector that is already pointing to a plugin (L79), so the only way to install it would be to first uninstall the plugin that causes the collision. There could also exist the option of redeploy the plugin that the user is trying to install but changing the colliding selectors to others that don't exist in the methods mapping.