Fractional v2 contest - slywaters's results

Lines of code

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

Vulnerability details

Impact

The Vault.sol contract is vulnerable and could have its ownership changed in the worst case. This is possible due to a dangerous use of the delegatecall function.

When a contract calls another via this function, the called function is able to execute its logic using the caller's storage. This is a security concern as variables controlling a contract's security may be modified by the called contract.

Line 132 contains a protection mechanism to revert the transaction if the contract's owner is changed in such a way.

However, this protection is insufficient as there is no mechanism to prevent the modification of other variables in the Vault.sol contract.

In particular, the nonce variable is responsible for the contract's security and may be modified.

Proof-of-concept

Review the init() function within the Vault.sol contract.

function init() external {
    if (nonce != 0) revert Initialized(owner, msg.sender, nonce);
    nonce = 1;
    owner = msg.sender;
    emit TransferOwnership(address(0), msg.sender);
}

This function can be used to change the ownership of the contract. It is protected by the nonce variable. The intention appears to be that the ownership may only be changed once and that this occurs when the contract is created by VaultFactory.sol.

However, a malicious contract called by delegatecall could attack Vault.sol by changing the value of nonce to 0. They could then make a call to init() as it is accessible externally. Once nonce == 0, they will gain complete ownership of the contract by calling init().

It is important to note that there is a control mechanism limiting when delegatecall can be used. Only the owner of Vault.sol can add "plugins" (contracts called by delegatecall) to an allow-list of approved contracts.

However, given that the code already contains a mechanism to prevent a change in ownership via delegatecall, it is implicit that plugins should not have any control over the ownership of Vault.sol. Because the nonce variable can also lead to a change in ownership, it should also be protected.

Furthermore, it is possible that the value of nonce could change simply by accident rather than through a deliberate attack. Here is the layout of the storage variables in the Vault.sol contract:

contract Vault is IVault, NFTReceiver {
    /// @notice Address of vault owner
    address public owner;
    /// @notice Merkle root hash of vault permissions
    bytes32 public merkleRoot;
    /// @notice Initializer value
    uint256 public nonce;
    /// @dev Minimum reserve of gas units
    uint256 private constant MIN_GAS_RESERVE = 5_000;
    /// @notice Mapping of function selector to plugin address
    mapping(bytes4 => address) public methods;

The location of the nonce variable will be in the 3rd "slot" of the called contract's storage. Any plugin that happens to modify this value to 0 for any purpose could allow for init() to be called.

An attacker who discovers this vulnerability could simply monitor the status of vulnerability and wait for nonce to become 0 during the lifecycle of the Vault.sol contract and call init() at that point.

In this way they could take ownership of the contract without any influence over the administrator or any control of the plugin code.

Tools Used

Reading code

Recommended Mitigation Steps

Add similar checks to nonce as the ones in place to verify the integrity of the owner variable.

It is worth considering additional protections for merkleRoot and methods as they are also related to the contract's authorization functionality.

See the link below for more information:

https://github.com/ethereumbook/ethereumbook/blob/develop/09smart-contracts-security.asciidoc#delegatecall

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

Impact

This function is deprecated and it can cause unexpected failures under specific conditions. In particular, when the total usage of the function call exceeds 2300 gas, it will fail.

Proof of Concept

A revert on transfer can occur when a contract is called via a proxy or when a payable fallback function is complex enough to exceed this gas usage threshold. .transfer can also fail if the claimer smart contract does not implement a payable function.

The following function calls are affected:

2022-07-fractional/src/modules/Migration.sol:172:        payable(msg.sender).transfer(ethAmount);
2022-07-fractional/src/modules/Migration.sol:325:        payable(msg.sender).transfer(userEth);

For more information, consult the following reference:

• Consensys - "Stop Using Solidity's Transfer Now"
• Solidity Docs. Compare the gas limitations between transfer and call

Tools Used

Editor

Recommended Mitigation Steps

It is advised to instead use the .call function on addresses that are payable.

Gas Report

Variables do not need to be initialized with 'empty' values such as 0, false, or address(0)

Uninitialized variables by default contain a value equivalent to 0: uints are initialized to 0; bools to false; addresses to address(0).

Explicitly assigning these values to variables when they are declared increases gas costs while providing no funciton.

e.g. change this code:

uint256 var = 0;

to

uint256 var;

For more information, please consult the following resources:

Tips and Tricks to Save Gas and Reduce Bytecode Size

The following lines of code are affected:

2022-07-fractional/src/Vault.sol:104:        for (uint256 i = 0; i < length; i++) {
2022-07-fractional/src/Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
2022-07-fractional/src/modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
2022-07-fractional/src/modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) {
2022-07-fractional/src/modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) {
2022-07-fractional/src/utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {

Unchecked increment can be used in for-loop

Newer versions of the Solidity compiler will check for integer overflows and underflows automatically. This provides safety but increases gas costs.

When an unsigned integer is guaranteed to never overflow, the unchecked feature of Solidity can be used to save gas costs.

A common case for this is for-loops using a strictly-less-than comparision in their conditional statement, e.g.:

uint256 length = someArray.length;
for (uint256 i; i < length; ++i) {
}

In cases like this, the maximum value for length is 2**256 - 1. Therefore, the maximum value of i is 2**256 - 2 as it will always be strictly less than length.

This example can be replaced with the following construction to reduce gas costs:

for (uint i = 0; i < length; i = unchecked_inc(i)) {
    // do something that doesn't change the value of i
}

function unchecked_inc(uint i) returns (uint) {
    unchecked {
        return i + 1;
    }
}

For more information, consult the following resources:

Solidity gas optimizations

Solidity docs: underflows, overflows, and unchecked

There are some instances in other files in the codebase already using this pattern. The following lines of code can be changed to match:

2022-07-fractional/src/Vault.sol:104:        for (uint256 i = 0; i < length; i++) {
2022-07-fractional/src/Vault.sol:78:        for (uint256 i = 0; i < length; i++) {

Replace postfix increment (var++) with prefix increment (++var)

Using ++i costs less gas than using i++. In the context of a for-loop, gas is saved on each iteration.

The following lines of code are affected:

2022-07-fractional/src/Vault.sol:104:        for (uint256 i = 0; i < length; i++) {
2022-07-fractional/src/Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
2022-07-fractional/src/modules/Migration.sol:508:                    hashes[counter++] = leaves[j];
2022-07-fractional/src/modules/protoforms/BaseVault.sol:133:                    hashes[counter++] = leaves[j];
2022-07-fractional/src/utils/MerkleBase.sol:188:                ceil++;

Array length can be cached

In the context of a for-loop that iterates over an array, it costs less gas to cache the array's length in a variable and read from this variable rather than use the arrays .length property. Reading the .length property for on the array will cause a recalculation of the array's length on each iteration of the loop which is a more expensive operation than reading from a stack variable.

For example, the following code:

for (uint i; i < arr.length; ++i) {
	// ...
}

should be changed to:

uint length = arr.length;
for (uint i; i < length; ++i) {
	// ...
}

Note that in the second case, the length of the array must not change during the loop's execution.

For more information, see the following resource:

Solidity gas optimizations

The following lines of code are affected:

2022-07-fractional/src/modules/Buyout.sol:454:        for (uint256 i; i < permissions.length; ) {
2022-07-fractional/src/modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
2022-07-fractional/src/modules/protoforms/BaseVault.sol:130:            for (uint256 i; i < _modules.length; ++i) {
2022-07-fractional/src/modules/protoforms/BaseVault.sol:132:                for (uint256 j; j < leaves.length; ++j) {
2022-07-fractional/src/modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) {
2022-07-fractional/src/modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) {
2022-07-fractional/src/utils/MerkleBase.sol:110:            for (uint256 i; i < result.length; ++i) {
2022-07-fractional/src/utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {

Use strict less-than/greather-than comparisons rather than less-than-or-equal/greater-than-or-equal

When compiled, Solidity code using the >= or <= comparison operators in fact executes two separate checks: one for 'is-equal-to' and a second for 'is-greater-than/is-less-than'. By contrast, using > or < performs only one check. Therefore code that is written to use strict comparison operators is more gas-efficient.

If this change is applied, be sure to update the relevant variables being evaluated. For clarity, it is also advised to rename the variables to make this change explicit, e.g. renaming a variable from MINIMUM to MINIMUM_PLUS_ONE.

The following lines are affected:

2022-07-fractional/src/modules/Buyout.sol:203:        if (block.timestamp <= endTime)