Samczsun talk about Re-entrancy

where is external call

msg.sender.call(hex"")

UniswapExchangeLiks(exchange).sync();
validate the address

ERC20like(token).decimals();

can the call be hooked

is there a way for an attacker to gain control of your execution

contract Caller{
    fallback() external{
        call...
    }
}

contract BadToken{
    function decimals() public returns(uint8) {
        //version < 0.5.0 call view -> use call; version > 0.5.0 call view -> use staticcall
    }
}

address is valide, but the call still can be hookable
ERC777
transfer call can also be hooked

three bad patterns

the only difference between an attacker doing something in an external call and attacker do something before calling your contract is they are now in the middle of execution.

whatever an attacker does they have to take advantage of the fact that they are in the middle of execution

1st pattern: state write before external call

function example() public {
    // some state update happens here
    msg.sender.call(hex"");
}

for this example, before the obvious external call, there is state change. the state update doesn't exists before the example function was called, therefore the attacker might be able to take advantage of that inside the external call to msg.sender

2nd pattern: state read after external call

function example() public {
    // unimportant
    msg.sender.call(hex"");
    // data read happens here
}

after the external call take place, there is a data read happens. if you image that the external call is not bullishes, the data read will perform some logic. maybe it reads current balance of the address. if in the external call the attacker can manipulate the data that read, it is possible that the attacker can cause the function to do that otherwise could have be done. and that is vulnerbility

3rd pattern: state read before external call and state write after external call

function example() public {
    // data read before the call
    msg.sender.call(hex"");
    // data write after the call
}

you have some data read before the call, the call happens, and the data is updated. this pattern indicates that the data read is used for some sort of validation. so your call happens after the validation, but before the logic to update state, you can re-enter the function as many times you want, that can bypass the validation

can you take advantage of it

to take advantage of the unsafe external call really depends on how much the entire ecosystem you understand. because now in the world composable, it is possible to attack one contract by manipulating another contract.

example 1:

contract SimpleBank{
    mapping(address=>uint) balances;
    function withdraw(uint amount) public {
    //make sure the user has enough deposited
        require(balances[msg.sender] >= amount);
        //give them their ether
        (bool ok, ) = msg.sender.call{value: amount}(hex"");
        //they got the ether, update our records
        if (ok) {
            balances[msg.sender] -= amount;
        }
    }
}

we see there is a withdraw function, which allows users to withdraw their ETH from the bank. very quickly, we noticed there is an external call, and we know that the call can be hooked. we recognize that the 3rd pattern, that is to say there is data read before the call and data updated after the call occurs. and in terms of step four, thinking about how to make use of it. again, classic re-entrant, you just call the withdraw function in your external call.

this is a quick example to illustrate that this sort of new guideline still works for the existing examples

example 2:

contract flashloaner{
    function loan(uint amount) public{
        //read state
        uint balanceStart = address(this).balance;
        //external call
        FlashReceiver(msg.sender).receive{value: amount}();
        //read state again
        uint balanceEnd = address(this).balance;
        require(balanceEnd >= balanceStart);        
    }
    receive() external payable{}
}

here is an example that you know that follow the traditional "check-effect-interact" guidelines, you might arrive at the conclusion that this function maybe unsafe, because the check is happening after the interaction. but if you think about it, while there is an external call, and the call can be hooked, there is no state update happening before the call, there is state read after the call you can manipulate, but there is nothing you can do with it. and there is no data being read before and write after, so based on our new set of guidelines, this function doesn't contains unsafe external call.

example 3:

contract SimpleFarm{
    mapping(address => uint) balances;
    function balanceOfUnderlyingAssests() public view returns(uint) {
        return value;
    }
    function depositToken(ERC20like token, uint amount) public {
        //calculate the increase in value from the deposit
        uint valueBefore = balanceOfUnderlyingAssests();
        token.transferFrom(msg.sender, address(this), amount);
        uint valueAfter = balanceOfUnderlyingAssests();

        require(valueAfter > valueBefore);

        //credit the user
        balances[msg.sender] += valueAfter - valueBefore;
    }
    function depositTokens(ERC20like[] memory tokens, uint[] memory amounts) public {
        uint valueBefore = balanceOfUnderlyingAssests();
        for (uint i=0; i < tokens.length; i++) {
            tokens[i].transferFrom(msg.sender, address(this), amounts[i]);
        }
        uint valueAfter = balanceOfUnderlyingAssests();
        require(valueAfter > valueBefore);
        //credit user
        balances[msg.sender] += valueAfter - valueBefore;
    }
}

we have three functions of interest, the first function basically just calculate the balance of the underlying assets. so , you know pretend that sums up the user's balance of Die, USDC, USDT you know all the stable coins returns the total balance. then we have a deposit function, which takes any token and some amount in order to know how much the credit the user with, they take a snapshot value before it does an transfer, and snapshots value after, if there is a change value, then it credits the user with that change. the reason why we doing this in this way is because one we want to take care of fee tokens right. if a token is a fee token, like USDT turn on the fee, then the user might deposit 1000 USDT, but we only get, say, 900. we dont want to credit the user with 1000 USD with deposits, so we use a snapshot method. and then the other reason we doing this is because if the user deposits some junk tokens we dont want to credit them for that deposit, we want to reject that. and the snapshot method means that it will revert because no balance change occurred. in the next function, the depositTokens function, we see basically the same logic except there is a loop because it is more gas efficient.

so you can see that individually all these functions, they make sense right like, there doesn't appear to be anything inherently wrong with them the logic we checked. but there is something interesting happen here. if we start the step 1, identify the external call, we see there is two of them, two transferFroms. we really care about this one in the for loop. as for the step 2, the question can the call be hooked? it can be answer with the fact that there is no validation on the tokens, you can specify any tokens you want. including the token that the attacker themselves deployed. that malicious token presumably would not do anything except if the transferFrom function was called in which case it would perform some other unsafe calls. as for step 3, in terms of there is any bad patterns, we recognize that here we have some unread data which can be manipulated by transfer tokens into the contract. so there is bad pattern happening here. the final question you want to ask yourself is how do we take advantage of this. again, this is very some thinking needs to happen and there isn't like a checklist we can follow. in this case, if we think about it, the only way we can manipulate the unread data is by transferring tokens into the contract. obvious it is not great for us as an attacker if we just transfer tokens because we sort of give the contract tokens for free. what we really want is some way to transfer token in that we can be credit for. because in that case, we will get credit once with that token, and in this depositTokens function will be credit again, two times their value. if it sounds like i am describing the depositToken function in a really long-wide way. it is because i am. the depositToken function does exactly that we transfer tokens to contract and we get some credited inexchange. so the final attack here is, we call the depositTokens function with one single token we control, when the contract try to transfer that token, we then call depositToken with actual stable coin we'll be credit once for deposit in the depositToken function and then we'll be credit again in the depositTokens function.

the point of this example shows that even though individually each function looks secure and even though there doesn't appear to be the classic reentrancy vulnerable here with the "check-effect-interact" pattern, this code is still vulnerable to the unsafe external call.

exploit POC：

import "./simplefarm.sol";
contract hack is ERC20like {
    SimpleFarm public simpleFarm;
    constructor(address _simpleFarm) public {
        simpleFarm = SimpleFarm(_simpleFarm);
    }
    function transferFrom(address _from, address _to, uint amount) public {
        simpleFarm.depositToken(USDC, amount);
    }
    function exploit() public {
        ERC20like[] memory tokens = new ERC20like[](1);
        tokens[0] = address(this);
        uint[] memory amounts = new uint[](1);
        amounts[0] = 1e18;
        simpleFarm.depositTokens(tokens,amounts);
    }
}

example 4:

function joinPool(uint poolAccountOut, uint[] calldata maxAmountsIn) external lock{
    uint[] memory actualAmountsIn = calculateAmountsIn(bPool, poolAmountsOut, maxAmountsIn);
    address[] memory poolTokens = bPool.getCurrentTokens();
    for (uint i=0; i < poolTokens.length; i++) {
        address t = poolTokens[i];
        uint tokenAmountIn = actualAmountsIn[i];
        emit LogJoin(t, msg.sender, tokenAmountIn);
        _pullUnderlying(t, msg.sender, tokenAmountIn);

    }
    _mintPoolShare(poolAmountOut);
    _pushPoolShare(msg.sender, poolAmountOut);
}
function _pullUnderlying(address erc20, address from, uint amount) internal {
    //get current balance of token1, B1, and weight of token1 from BPool, W1
    uint tokenBalance = bPool.getBalance(erc20);
    uint tokenWeight = bPool.getDenormalizeWeight(erc20);

    bool xfer = IERC20(erc20).transferFrom(from, address(this), amount);
    require(xfer);
    bPool.rebind(erc20, BalancerSafeMath.badd(tokenBalance, amount), tokenWeight);
}

this is code that taken from a contract that built on top of a balancer pool. for those one who doesn't fallimer, balancer is basicaller like Uniswap, but you can have more than 2 tokens. Due to some technical limitations, the developer have to do is to re-implement a bunch of functions that the balancer pool already provided. here they re-implement the join pool function. the join pool function allows user to deposit tokens that the pool supports and the pool tokens in return. it allows user to supply liquidity. so first, the users specify how many pool tokens they want, the contract calculate the amount of underlying tokens the user needs to deposit. and then for each token, the contract transfer it from the user to the contract and then from contract to the underlying balancer pool. once all the transfer have taken place, the contract then mints pool tokens. so to see sort of visually what's happening here, if we take for example, this balancer pool which contains Dai, USDC, USDT, imBTC. if we wants to join the pool, and mint some pool tokens, we will need to deposit a perpotional amount of tokens for all four underlying tokens. right, so you see all of four goes up., in equal like underlying value not absolute token balance.

so you know, overall this code looks pretty secure, if we assume that the underlying balancer pool secure. this function looks fine, eagle observers might also notice that this function has a lock modifier which means that you can not reentrant this function if you wanted to.

but again, i am showing you this example here, it is clearly something interesting happening here. if we go through our process again, we can first notice that there is an external call happening - the transferFrom. as for this external call whether is hook-able, you may also notice that i include imBTC as a token in this pool. for those familiar with the De-Force hack, you will remember that imBTC implements the ERC777 standard, which means that this call here, although it is to a trusted address, in this case a good pool token, it still results to a callback to attacker right. now the question is : is there some way to exploit this callback , or first of all is there any bad patterns here. again, at first glance, it doesn't to be anything interesting, you know, we can run through our check list, is there a state modified before external call, or is there data read afterwards, and is there data that read before and updated afterwards. and you know, a very quickly glance will turn out nothing, but if you breaks it down into individual steps, you will notice that there actually is state updated before external call. if we go back to our pool, and pretend to deposit some tokens again but this time we run through step by step. well, first we have to transfer tokens in. right? we can not transfer all four tokens at a time, we have to transfer one by one. so first, we'll transfer the Dai, after the transferring Dai, we are about to transfer the imBTC. when the attacker receives a callback. now you may notice that at this moment time, not once the entire join pool transfer finished, at the moment of time, during the execution of join pool, the pool is unbalanced. there is more Dai in the pool than there is USDC and USDT. and balancer incentivise uses to bring the pool back into balance right. so in our callback that we received from imBTC token, we can perform a swap on the underlying pool, buying Dai using USDC at a muck lower price than we normally would have been able to.

and so the point of this example showing that, there does not appear to be an reentrancy vulnerablity in this, you know new contract, and there is no bugs in the underlying balancer pool. and there doesn't appear to be anything wrong with the code, there is still bad pattern and there was still an unsafe external call.

imBTC-EIP 777

deposit dai first, bring the pool unbalanced

![image-20210801210143415](image/![image20210801210143415.png]

take effects of imBTC's callback, we can swap dai to usdc in this underlying pool

validate contract address receive from users

tx.origin == msg.sender

extcodesize(msg.sender)