creamfi / balancer-core Goto Github PK

1. Deploy the contract to Ropsten.
2. Verify the contract on etherscan.
3. Register thegraph for the deployed contract.
4. Modify our frontend code to fetch data from our thegraph API.

What is wrong?

https://hackmd.io/mm0PhJ-YR8euWs6vSGinAQ#Reserves-and-balance-mismatch-ERC20balanceOf

How to fix it?

• Change to _records[token].balance = IERC20(token).balanceOf(address(this)) - totalReserves[token]

What is wrong?

Error messages are removed to let us have more space to add new functions. Find out a way to add some messages back.

#18 中 slither failed。在這 PR 中，我們在 gulp 中對 IERC20(token).balanceOf(address(this)) 的結果運算，在看似無關的地方觸發了 slither 的 incorrect-equality 錯誤。

Error message

$ slither . --filter-paths "test" --exclude=naming-convention,unused-state,solc-version,constable-states,external-function,reentrancy-events

...
INFO:Detectors:
BNum.bdiv(uint256,uint256) (BNum.sol#75-86) uses a dangerous strict equality:
        - require(bool)(a == 0 || c0 / a == BONE) (BNum.sol#81)
BNum.bmul(uint256,uint256) (BNum.sol#63-73) uses a dangerous strict equality:
        - require(bool)(a == 0 || c0 / a == b) (BNum.sol#68)
BNum.bpow(uint256,uint256) (BNum.sol#108-126) uses a dangerous strict equality:
        - remain == 0 (BNum.sol#120)
BNum.bpowApprox(uint256,uint256,uint256) (BNum.sol#128-161) uses a dangerous strict equality:
        - term == 0 (BNum.sol#149)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-strict-equalities
INFO:Slither:. analyzed (16 contracts with 40 detectors), 4 result(s) found
INFO:Slither:Use https://crytic.io/ to get access to additional detectors and Github integration
...

試著 debug 後發現 slither 會偵測「有沒有 balance 在 require 中被用 == 比較」，因此我猜測有被丟到 BNum.bdiv 、 BNum.bmul 、 BNum.bpow 、及 BNum.bpowApprox 的參數，在這 PR 後被標記為是 balance。因為這些函數會對這些參數做像是 BNum.bmul 中 require(a == 0 || c0 / a == b) 這種違反 incorrect-equality 的行為。

目前我傾向先讓 slither 忽略 incorrect-equality 不檢查，因為要 debug 需要滿多時間的，然後感覺問題很可能是在 slither。

Reference

• slither 中做標記 balance 的 code。

In #1 , we have totalReserves storing all reserved fees. Eventually, we transfer all these tokens in all pools back to an address provided by BFactory.

Possible structure

BFactory

contract BFactory {
    ...
    function getReservesAddress() returns (address);
    // Can only be called by the admin who deployed this contract.
    function setReservesAddress(address reservesAddress);
    // Transfer the reserves tokens in `pool` to the `reservesAddress`.
    function collectTokens(BPool pool);
}

BPool

contract BPool {
    ...
    function withdrawReserves(address reserveAddress) {
        require(msg.sender == _factory);
        for (t in _tokens) {
            // transfer reserves tokens to `reserveAddress`
            ...
        }
    }
}

TODO

• Confirm reserves ratio: https://github.com/CreamFi/balancer-core/blob/master/contracts/BConst.sol#L27

TL;DR

Don't call BFactory.collect(BPool pool) because

1. This way we avoid any security concerns.
2. We don't need to call it anyway since the exit fee is zero.

What's wrong

BPool.drainTotalReserves should be secure in most of the condition thanks to the guard at

1. In BFactory.newBPool
   

   balancer-core/contracts/BFactory.sol

   Line 59 in f571d8f

   bpool.setController(msg.sender);

This is fine because bpool is created by BFactory and the code is known to us.

2. In BFactory.collect
   

   balancer-core/contracts/BFactory.sol

   Lines 123 to 124 in f571d8f

   	uint collected = IERC20(pool).balanceOf(address(this));
   	bool xfer = pool.transfer(_blabs, collected);

This is possibly manipulated if pool is a contract with malicious/malformed balanceOf or transfer. If it calls any pool's drainTotalReserves with delegatecall, totalReserves can be withdrawn to attackers address.

3. In BFactory.collectTokenReserves
   

   balancer-core/contracts/BFactory.sol

   Line 133 in f571d8f

   pool.drainTotalReserves(_reservesAddress);

This is fine because we have require(_isBPool[address(pool)]) guarding us, which is only passed through when pool is one of our pools.