Fractional v2 contest - simon135's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Buyout.sol#L86

Vulnerability details

percsion error that causes buyoutprice to be very low causing the fractionPrice very low

detials

how does buyout become realy low? because of this percison error in the code if you supply msg.value=1 depsitAmount=1 totalSupply=5

1. also there is a possiblity that the vault has no tokens left over then the function will revert. buyoutPrice = 1*100 / (100 -(1/5))=1 fractionPrice1/5=0 because the smart contract dosnt know the fraction price is lowerd you can sell your fraction tokens or more then there worth and then buy it back the fractions for 0.

     uint256 buyoutPrice = (msg.value * 100) /

            (100 - ((depositAmount * 100) / totalSupply));
  uint256 fractionPrice = buyoutPrice / totalSupply
   

you can do the same but with the price infilited if you are taking a flash loan on the fractionprice you can inflate it and make the fraction worth more and cause a aribtrary trade /flashloan to cause loss of funds instead ifts a very high amount then fractionPrice *amount =ethbalacne would be more then the attacker supplied causing loss of funds but if the token was low price then when

        if (msg.value != fractionPrice * _amount) revert InvalidPayment();


        // Transfers fractional tokens to caller
        IERC1155(token).safeTransferFrom(
            address(this),
            msg.sender,
            id,
            _amount,
            ""
        );

then you dont need to supply eth and just take as much tokens as possible with _amount=1232133333 large amount also if fractionprice is large price then users will have to supplly a large amount of eth that will dos the functions this in general percission errors causes the smart contract to brake

mitigation

fractionPrice = buyoutPrice *1000 /totalsupply just make sure decimals cover the percision lost. require (fractionprice>0) ;

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L172 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L325

Vulnerability details

details

To withdraw eth it uses transfer(), this trnansaction will fail inevitably when : -

The withdrwer smart contract does not implement a payable function.

Withdrawer smart contract does implement a payable fallback which uses more than 2300 gas unit

Thw withdrawer smart contract implements a payable fallback function whicn needs less than 2300 gas unit but is called through proxy that raise the call's gas usage above 2300

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

proof of concept

 payable(msg.sender).transfer(userEth);

mitigation

use call instead of transfer

no address zero check

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L93

this comments dosnt make any sense

 // Reverts if payment amount does not equal price of fractional amount

it should say instead of payment make it eth payment Buyout:164

missing return natspec

after this: Migration.sol:71: /// @param _targetPrice Target price of the buyout

in the exucte function the proof variable is memory and you can make it memory here to or make them both calldata

     address _to,

        uint256 _fractionSupply,

        bytes32[] calldata _mintProof

function execute(address _target, bytes memory _data, bytes32[] memory _proof) external payable returns (bool success, bytes memory response)

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Minter.sol#L54

init function can get frontruned and the contract will have to get redpolyed

function init() external {

        if (nonce != 0) revert Initialized(owner, msg.sender, nonce);

        nonce = 1;

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L24

make sure fallback/receive function has a a function init to make sure usersr to loose funds byaccident

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L32

no emiting of the last owner variable

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L93

no 2 step owner ship functions to make the newowner to approve to be owner

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L93

totalsupply wouldnt matter if the buyout is successful or not

as it says in the comments

   // Checks totalSupply of auction pool to determine if buyout is successful or not

this above is not true
it is very likely to pass even though tokenBalance is 0 and didnt get anything because the vault should have engouhg funds for 500 wei

          (tokenBalance * 1000) /

                IVaultRegistry(registry).totalSupply(_vault) >

            500

as long as there is little dust in the vault then it will pass even though most of the funds where tooken out.

mitigation

make sure tokenbalance is > 0 or have require statement that checks if the vault is in the middle of buyout or did it recently. https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Buyout.sol#L208

the amount of tokens they are transfering to you are not eth but a diffrent tokens which costs more causing loss of funds

     uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);

        uint256 buyoutShare = (tokenBalance * ethBalance) /

            (totalSupply + tokenBalance);

when doing the caluction you can have a token that is not eth or weht and more deicamls making it more eth getting transferd to you then you paid for

10*1 / 10+10=0 which in this case with small amount of money the user will loose 
2.
10*100/ 10+10=50 
 50 shares  but it can be worth more in weht/eth making it loss of funds

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Buyout.sol#L208

mitagtion

transfer those funds to him but convert to eth and and check it with a price orcal to make sure they are equal

use safetransfer for number of reasons better then transferfrom/transfer

because it can revert if transfering 0 or fail sliance if transfer dosnt go through protoforms/BaseVault.sol:65: IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);

attacker can minuplate the function and take the attacker ( his) left over funds but transferd from a diffrent vault not the one the attacker deposited to.

if      vault is random but real vault that fits the if statement

            proposalId  is ,some time ago you have left over Fractions and now you want to get it out even though your main vault had a succesful migration and you still have  an amount in it then that vault is loosing funds  because you had some value in another vault and it sends you the funds even though its a diffrent vault that migrited

 uint256 userFractions = userProposalFractions[_proposalId][msg.sender];
        // Updates fractional balance of caller
        userProposalFractions[_proposalId][msg.sender] = 0;
        // Withdraws fractional tokens from contract back to caller
        IFERC1155(token).safeTransferFrom(
            address(this),
            msg.sender,
            id,
            userFractions,
            ""
        );

proof of concept

so as long as you its a real vault that has funds but proposalId has some amount into from it an attacker can steal funds from another vault. same thing with eth to. https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Migration.sol#L292

you can have a dos becasue the 1 ether is static and you dont lett the user choose the calulation total

and the user dosnt choose so if a user dosnt have 1 ether and instead has a 10**5 wei then it will revert.

 uint256 totalInEth = _calculateTotal(

            1 ether,

            lastTotalSupply,

            migrationInfo[_vault][_proposalId].totalEth,

            migrationInfo[_vault][_proposalId].totalFractions

        );

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Migration.sol#L452

make string into bytes to save gas

FERC1155.sol:15:    string public constant NAME = "FERC1155";
FERC1155.sol:17:    string public constant VERSION = "1";

make constants private to save gas

ERC1155.sol:15:    string public constant NAME = "FERC1155";
FERC1155.sol:17:    string public constant VERSION = "1";
Buyout.sol:35:    uint256 public constant PROPOSAL_PERIOD = 2 days;
Buyout.sol:37:    uint256 public constant REJECTION_PERIOD = 4 days;
Migration.sol:43:    uint256 public constant PROPOSAL_PERIOD = 7 days;

cache array.lenght into a memory variable to save gas

protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) {
protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) {
protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
protoforms/BaseVault.sol:130:            for (uint256 i; i < _modules.length; ++i) {
protoforms/BaseVault.sol:132:                for (uint256 j; j < leaves.length; ++j) {
Buyout.sol:454:        for (uint256 i; i < permissions.length; ) {

make constants immutable to save gas

Buyout.sol:35:    uint256 public constant PROPOSAL_PERIOD = 2 days;
Buyout.sol:37:    uint256 public constant REJECTION_PERIOD = 4 days;
Migration.sol:43:    uint256 public constant PROPOSAL_PERIOD = 7 days;

make constuctor variabls ,immutable to save gas

Buyout.sol:29:    address public registry;
Buyout.sol:31:    address public supply;
Buyout.sol:33:    address public transfer;
Migration.sol:37:    address payable public buyout;
Migration.sol:39:    address public registry;

cache storage mappings or arrays into memory

buyoutInfo[_vault].ethBalance += msg.value;
@audit instead use ethbalance=buyoutInfo[_vault].ethBalance
@audit ethbalacne+=msg.value saving ssload 

use abi.encodepacked instead of abi.encode

Buyout.sol:456:            nodes[i] = keccak256(abi.encode(permissions[i]));
Minter.sol:26:        nodes[0] = keccak256(abi.encode(getPermissions()[0]));

make address(0) the long way

0x0000000000000 instead to save gas Migration.sol:228: if (proposal.newVault != address(0)) Migration.sol:267: if (proposal.newVault == address(0)) revert NoVaultToMigrateTo(); Migration.sol:304: migrationInfo[_vault][_proposalId].newVault != address(0)

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled. i++ increments i and returns the initial value of i. Which means: uint i = 1; i++; // == 1 but i == 2 But ++i returns the actual incremented value: uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/utils/MerkleBase.sol#L189

make only owner functions payable to save gas

becauser payble dosnt need msg.value check of 0 https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L93 https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L73-L101