DAOfi

Remove inline comments that suggest the two uint256 values DAOfiV1Pair.reserveBase and DAOfiV1Pair.reserveQuote are stored in the same storage slot. This is likely a carryover from the UniswapV2Pair contract, in which reserve0, reserve1, and blockTimestampLast are packed into a single storage slot.

code/daofi-v1-core/contracts/DAOfiV1Pair.sol:L44-L45

uint256 private reserveBase;       // uses single storage slot, accessible via getReserves
uint256 private reserveQuote;      // uses single storage slot, accessible via getReserves

The DAOfiV1Pair functions initialize(), getBaseOut(), and getQuoteOut() all use the private function _getFormula(), which makes a call to the factory to retrieve the address of the BancorFormula contract. The formula address in the factory is set in the constructor and cannot be changed, so these calls can be replaced with an immutable value in the pair contract that is set in its constructor.

code/daofi-v1-core/contracts/DAOfiV1Pair.sol:L94-L96

function _getFormula() private view returns (IBancorFormula) {
    return IBancorFormula(IDAOfiV1Factory(factory).formula());
}

DAOfiV1Pair should not be used with rebasing tokens – that is, tokens in which an account’s balance increases or decreases along with expansions or contractions in supply. The contract provides no mechanism to update its reserves in response to these unexpected balance adjustments, and funds may be lost as a result.

DAOfiV1Router01 should not be used with fee-on-transfer tokens – that is, tokens in which the balance of the recipient of a transfer may not be increased by the amount of the transfer. Some router functions make strict checks on the resulting balances and would fail for such tokens.

The development team has acknowledged these limitations, and it is recommended to continue to ensure that users are aware of them as well.

Description

DAOfiV1Router01.addLiquidity() creates the desired pair contract if it does not already exist, then transfers tokens into the pair and calls DAOfiV1Pair.deposit(). There is no validation of the address to transfer tokens from, so an attacker could pass in any address with nonzero token approvals to DAOfiV1Router. This could be used to add liquidity to a pair contract for which the attacker is the pairOwner, allowing the stolen funds to be retrieved using DAOfiV1Pair.withdraw().

code/daofi-v1-periphery/contracts/DAOfiV1Router01.sol:L57-L85

function addLiquidity(
    LiquidityParams calldata lp,
    uint deadline
) external override ensure(deadline) returns (uint256 amountBase) {
    if (IDAOfiV1Factory(factory).getPair(
        lp.tokenBase,
        lp.tokenQuote,
        lp.slopeNumerator,
        lp.n,
        lp.fee
    ) == address(0)) {
        IDAOfiV1Factory(factory).createPair(
            address(this),
            lp.tokenBase,
            lp.tokenQuote,
            msg.sender,
            lp.slopeNumerator,
            lp.n,
            lp.fee
        );
    }
    address pair = DAOfiV1Library.pairFor(
        factory, lp.tokenBase, lp.tokenQuote, lp.slopeNumerator, lp.n, lp.fee
    );

    TransferHelper.safeTransferFrom(lp.tokenBase, lp.sender, pair, lp.amountBase);
    TransferHelper.safeTransferFrom(lp.tokenQuote, lp.sender, pair, lp.amountQuote);
    amountBase = IDAOfiV1Pair(pair).deposit(lp.to);
}

Recommendation

Transfer tokens from msg.sender instead of lp.sender.

Description

To create a new pair, a user is expected to call the same addLiquidity() (or the addLiquidityETH()) function of the router contract seen above:

code/daofi-v1-periphery/contracts/DAOfiV1Router01.sol:L57-L85

function addLiquidity(
    LiquidityParams calldata lp,
    uint deadline
) external override ensure(deadline) returns (uint256 amountBase) {
    if (IDAOfiV1Factory(factory).getPair(
        lp.tokenBase,
        lp.tokenQuote,
        lp.slopeNumerator,
        lp.n,
        lp.fee
    ) == address(0)) {
        IDAOfiV1Factory(factory).createPair(
            address(this),
            lp.tokenBase,
            lp.tokenQuote,
            msg.sender,
            lp.slopeNumerator,
            lp.n,
            lp.fee
        );
    }
    address pair = DAOfiV1Library.pairFor(
        factory, lp.tokenBase, lp.tokenQuote, lp.slopeNumerator, lp.n, lp.fee
    );

    TransferHelper.safeTransferFrom(lp.tokenBase, lp.sender, pair, lp.amountBase);
    TransferHelper.safeTransferFrom(lp.tokenQuote, lp.sender, pair, lp.amountQuote);
    amountBase = IDAOfiV1Pair(pair).deposit(lp.to);
}

This function checks if the pair already exists and creates a new one if it does not. After that, the first and only deposit is made to that pair.

The attacker can front-run that call and create a pair with the same parameters (thus, with the same address) by calling the createPair function of the DAOfiV1Factory contract. By calling that function directly, the attacker does not have to make the deposit when creating a new pair. The initial user will make this deposit, whose funds can now be withdrawn by the attacker.

Recommendation

There are a few factors/bugs that allowed this attack. All or some of them should be fixed:

• The createPair function of the DAOfiV1Factory contract can be called directly by anyone without depositing with any router address as the parameter. The solution could be to allow only the router to create a pair.
• The addLiquidity function checks that the pair does not exist yet. If the pair exists already, a deposit should only be made by the owner of the pair. But in general, a new pair shouldn’t be deployed without depositing in the same transaction.
• The pair’s address does not depend on the owner/creator. It might make sense to add that information to the salt.

Description

The _convert() function in DAOfiV1Pair is used to accommodate tokens with varying decimals() values. There are three cases in which it implicitly returns 0 for any amount, the most notable of which is when token.decimals() == resolution.

As a result of this, getQuoteOut() reverts any time either baseToken or quoteToken have decimals == INTERNAL_DECIMALS (currently hardcoded to 8).

getBaseOut() also reverts in most cases when either baseToken or quoteToken have decimals() == INTERNAL_DECIMALS. The exception is when getBaseOut() is called while supply is 0, as is the case in deposit(). This causes getBaseOut() to succeed, returning an incorrect value.

The result of this is that no swaps can be performed in one of these pools, and the deposit() function will return an incorrect amountBaseOut of baseToken to the depositor, the balance of which can then be withdrawn by the pairOwner.

code/daofi-v1-core/contracts/DAOfiV1Pair.sol:L108-L130

function _convert(address token, uint256 amount, uint8 resolution, bool to) private view returns (uint256 converted) {
    uint8 decimals = IERC20(token).decimals();
    uint256 diff = 0;
    uint256 factor = 0;
    converted = 0;
    if (decimals > resolution) {
        diff = uint256(decimals.sub(resolution));
        factor = 10 ** diff;
        if (to && amount >= factor) {
            converted = amount.div(factor);
        } else if (!to) {
            converted = amount.mul(factor);
        }
    } else if (decimals < resolution) {
        diff = uint256(resolution.sub(decimals));
        factor = 10 ** diff;
        if (to) {
            converted = amount.mul(factor);
        } else if (!to && amount >= factor) {
            converted = amount.div(factor);
        }
    }
}

Recommendation

The _convert() function should return amount when token.decimals() == resolution. Additionally, implicit return values should be avoided whenever possible, especially in functions that implement complex mathematical operations.

BancorFormula.power(baseN, baseD, _, _) does not support baseN < baseD, and checks should be added to ensure that any call to the BancorFormula conforms to the expected input ranges.

Description

The following lines are intended to check that the amount of tokens received from a swap is greater than the minimum amount expected from this swap (sp.amountOut):

code/daofi-v1-periphery/contracts/DAOfiV1Router01.sol:L341-L345

uint amountOut = IWETH10(WETH).balanceOf(address(this));
require(
    IWETH10(sp.tokenOut).balanceOf(address(this)).sub(balanceBefore) >= sp.amountOut,
    'DAOfiV1Router: INSUFFICIENT_OUTPUT_AMOUNT'
);

Instead, it calculates the difference between the initial receiver’s balance and the balance of the router.

Recommendation

Check the intended value.

Description

DAOfiV1Pair.deposit() is used to deposit liquidity into the pool. Only a single deposit can be made, so no liquidity can ever be added to a pool where deposited == true. The deposit() function does not check for a nonzero deposit amount in either token, so a malicious user that does not hold any of the baseToken or quoteToken can lock the pool by calling deposit() without first transferring any funds to the pool.

code/daofi-v1-core/contracts/DAOfiV1Pair.sol:L223-L239

function deposit(address to) external override lock returns (uint256 amountBaseOut) {
    require(msg.sender == router, 'DAOfiV1: FORBIDDEN_DEPOSIT');
    require(deposited == false, 'DAOfiV1: DOUBLE_DEPOSIT');
    reserveBase = IERC20(baseToken).balanceOf(address(this));
    reserveQuote = IERC20(quoteToken).balanceOf(address(this));
    // this function is locked and the contract can not reset reserves
    deposited = true;
    if (reserveQuote > 0) {
        // set initial supply from reserveQuote
        supply = amountBaseOut = getBaseOut(reserveQuote);
        if (amountBaseOut > 0) {
            _safeTransfer(baseToken, to, amountBaseOut);
            reserveBase = reserveBase.sub(amountBaseOut);
        }
    }
    emit Deposit(msg.sender, reserveBase, reserveQuote, amountBaseOut, to);
}

Recommendation

Require a minimum deposit amount in both baseToken and quoteToken, and do not rely on any assumptions about the distribution of baseToken as part of the security model.

Description

The DAOfiV1Pair functions deposit(), withdraw(), and swap() are all restricted to calls from the router in order to avoid losses from user error. However, this means that any unidentified issue in the Router could render all pair contracts unusable, potentially locking the pair owner’s funds.

Additionally, DAOfiV1Factory.createPair() allows any nonzero address to be provided as the router, so pairs can be initialized with a malicious router that users would be forced to interact with to utilize the pair contract.

code/daofi-v1-core/contracts/DAOfiV1Pair.sol:L223-L224

function deposit(address to) external override lock returns (uint256 amountBaseOut) {
    require(msg.sender == router, 'DAOfiV1: FORBIDDEN_DEPOSIT');

code/daofi-v1-core/contracts/DAOfiV1Pair.sol:L250-L251

function withdraw(address to) external override lock returns (uint256 amountBase, uint256 amountQuote) {
    require(msg.sender == router, 'DAOfiV1: FORBIDDEN_WITHDRAW');

code/daofi-v1-core/contracts/DAOfiV1Pair.sol:L292-L293

function swap(address tokenIn, address tokenOut, uint256 amountIn, uint256 amountOut, address to) external override lock {
    require(msg.sender == router, 'DAOfiV1: FORBIDDEN_SWAP');

Recommendation

Do not restrict DAOfiV1Pair functions to calls from router, but encourage users to use a trusted router to avoid losses from user error. If this restriction is kept, consider including the router address in the deployment salt for the pair or hardcoding the address of a trusted router in DAOfiV1Factory instead of taking the router as a parameter to createPair().

Description

The parameters used to define a unique pair are the baseToken, quoteToken, slopeNumerator, n, and fee. There is only one accepted value for n, and there are eleven accepted values for fee. This makes the number of possible “interesting” pools for each token pair somewhat limited, and pools can be easily blocked by front-running deployments and depositing zero liquidity or immediately withdrawing deposited liquidity. Because liquidity can only be added once, these pools are permanently blocked.

The existing mitigation for this issue is to create a new pool with slightly different parameters. This creates significant cost for the creator of a pair, forces them to deploy a pair with sub-optimal parameters, and could potentially block all interesting pools for a token pair.

The salt used to determine unique pair contracts in DAOfiV1Factory.createPair():

code/daofi-v1-core/contracts/DAOfiV1Factory.sol:L77-L84

require(getPair(baseToken, quoteToken, slopeNumerator, n, fee) == address(0), 'DAOfiV1: PAIR_EXISTS'); // single check is sufficient
bytes memory bytecode = type(DAOfiV1Pair).creationCode;
bytes32 salt = keccak256(abi.encodePacked(baseToken, quoteToken, slopeNumerator, n, fee));
assembly {
    pair := create2(0, add(bytecode, 32), mload(bytecode), salt)
}
IDAOfiV1Pair(pair).initialize(router, baseToken, quoteToken, pairOwner, slopeNumerator, n, fee);
pairs[salt] = pair;

Recommendation

Consider adding additional parameters to the salt that defines a unique pair, such as the pairOwner. Modifying the parameters included in the salt can also be used to partially mitigate other security concerns raised in this report.

Description

While the rest of the system uses the safeTransfer* pattern, allowing tokens that do not return a boolean value on transfer() or transferFrom(), DAOfiV1Router01.removeLiquidityETH() throws and consumes all remaining gas if the base token does not return true.

Note that the deposit in this case can still be withdrawn without unwrapping the Eth using removeLiquidity().

code/daofi-v1-periphery/contracts/DAOfiV1Router01.sol:L157-L167

function removeLiquidityETH(
    LiquidityParams calldata lp,
    uint deadline
) external override ensure(deadline) returns (uint amountToken, uint amountETH) {
    IDAOfiV1Pair pair = IDAOfiV1Pair(DAOfiV1Library.pairFor(factory, lp.tokenBase, WETH, lp.slopeNumerator, lp.n, lp.fee));
    require(msg.sender == pair.pairOwner(), 'DAOfiV1Router: FORBIDDEN');
    (amountToken, amountETH) = pair.withdraw(address(this));
    assert(IERC20(lp.tokenBase).transfer(lp.to, amountToken));
    IWETH10(WETH).withdraw(amountETH);
    TransferHelper.safeTransferETH(lp.to, amountETH);
}

Recommendation

Be consistent with the use of safeTransfer*, and do not use assert() in cases where the condition can be false.