Sherlock contest - walker's results

Handle

walker

Vulnerability details

type: Denial of Service severity: High

A problem exists in the poolbase logic which calls LibPool.payOffDebtAll(_token) in the withdrawProtocolBalance() function. https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/PoolBase.sol#L285

This call will fail if one of the protocols in the respective pool has an insufficient balance to pay off their debt. The following line will revert when this is the case: https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/libraries/LibPool.sol#L103

As a result, it's necessary for in debt protocols to be cleaned before protocols can withdraw their balance. The issue is worsened by the fact that a protocol can keep idling around non-zero debt. An attacker can prevent a protocol from being "cleaned" by frontrunning any clean transaction and topping up their balance.

This gives an attacker the capability to make it difficult for protocols to extract their funds. I should note that the govMain account can trivially cause the same DoS by not acting when a protocol goes into debt.

A potential solution involves a change to the functions so that an actor will pay only the protocols' own debt.

Handle

walker

Vulnerability details

PoolBase enables an easy withdrawal of all funds

severity: critical type: memory safety

Description

A memory safety bug in the pool base allows participants to trick the system into believing they're interacting with a pool's token. While in reality, they're using an arbitrary (potentially just created token). As a result, actors such as protocol agents can drain the funds of the smart contract.

Details

The vulnerability is caused by the code in the function bps. https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/PoolBase.sol#L432-L441

This code fetches the last calldata entry, where the token address, which is being interacted with, should be located. Unfortunately, the abidecoder v2 allows for data after the regular arguments, allowing us to control bps() and baseData() without affecting regular execution.

// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.7.4;
pragma abicoder v2;


contract Test {
    fallback() external payable {}
    
    function help(address a) external returns (bool) {
        bytes memory array = msg.data;
        uint256 index = msg.data.length;
        address rt;
        // solhint-disable-next-line no-inline-assembly
        assembly {
          // Load the 32 bytes word from memory with the address on the lower 20 bytes, and mask those.
          rt := and(mload(add(array, index)), 0xffffffffffffffffffffffffffffffffffffffff)
        }
        return rt == a;
    }
}

/// run with calldata: 0x22646c93 + address a + address b

Attack

This affects a range of behaviours. I'll discuss one exploit chain which leverages this capability.

First, assume a protocol in the system which has agent Bob. Agent bob will perform three actions.

1. Bob will create a new ERC20 with 100 tokens minted to his address, and he'll approve the sherlock diamond to transfer those tokens.
2. Bob will call depositProtocolBalance(bobsProtocol, 100, newToken) and append the address of the USDC token to the calldata
3. Now bob will call withdrawProtocolBalance(bobsProtocol, 100, bob, USDC)

This scenario assumes that there is a pool for USDC and that the system holds at least 100 tokens

Explanation

This attack works because the function depositProtocolBalance() assumes that _token is the token for the basedata ps: https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/PoolBase.sol#L260-L272

Unfortunately, this is not the case because we were able to manipulate bps into returning the pool storage for the USDC pool. This function will then continue to move 100 random tokens into the sherlock contract and record that bob will now have 100 USDC worth of tokens in the Sherlock system.

Remediation

The current approach abstracts away getting the pool for a particular token. Removing this abstraction and explicitly fetching the pool storage for the token interacted with would likely remove this vulnerability.

Handle

walker

Vulnerability details

type: Incorrect Assumptions on External Systems

The sherlock smart contract system uses internal bookkeeping of arbitrary ERC20 token balances. It doesn't assert that the ERC20 doesn't implement some non-standard behaviour. For example, deflationary tokens, or tokens with a transfer fee, will result in incorrect internal balances. In summary, an attacker can perform stake and deposit actions without actually depositing the amount that sherlock assumes. As a result, an attacker is unduly rewarded balance and yield.

Balancer had a similar vulnerability in their system https://blog.1inch.io/balancer-hack-2020-a8f7131c980e.

An example location where such internal bookkeeping happens can be found here: https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/PoolBase.sol#L271

Mitigating the issue is possible by requiring the amount to be added to the contracts' balance. Alternatively, it's possible to update the pool based on actual balance changes.