Contracts Managed and ERC2771Context are incompatible, but both are inherited by AddressDriver and NFTDriver

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/AddressDriver.sol#L19 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/NFTDriver.sol#L19 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Managed.sol#L47 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Managed.sol#L54

Vulnerability details

Impact

Functions pause(), unpause(), changeAdmin(), grantPauser(), revokePauser() will always fail if called via a forwarder(e.g. Caller)

Proof of Concept

RC2771Context is an implementation of Context. The contract can extract the sender of a forwarded transaction from the calldata, which enables users to call the contract through a trustedForwarder.

function _msgSender() internal view virtual override returns (address sender) {
    if (isTrustedForwarder(msg.sender)) {
        // The assembly code is more direct than the Solidity version using `abi.decode`.
        /// @solidity memory-safe-assembly
        assembly {
            sender := shr(96, calldataload(sub(calldatasize(), 20)))
        }
    } else {
        return super._msgSender();
    }
}

AddressDriver and NFTDriver both inherit from ERC2771Context, so its functions can be called from forwarder, _msgSender() is used instead of msg.sender:

function _transferFromCaller(IERC20 erc20, uint128 amt) internal {
    erc20.safeTransferFrom(_msgSender(), address(this), amt);
    // Approval is done only on the first usage of the ERC-20 token in DripsHub by the driver
    if (erc20.allowance(address(this), address(dripsHub)) == 0) {
        erc20.safeApprove(address(dripsHub), type(uint256).max);
    }
}

However, AddressDriver and NFTDriver also inherit from Managed, which does not support forwarder and always uses msg.sender directly:

modifier onlyAdmin() {
    require(admin() == msg.sender, "Caller is not the admin");
    _;
}

This leads to a bit of a mess between AddressDriver and NFTDriver, with one part supporting the forwarder and the other not.

Tools Used

VS Code

Recommended Mitigation Steps

We should make the Managed contract inherit from Context and replace all msg.sender with _msgSender() in it.

[GalloDaSballo]

Looks off, some tx are setup for meta-tx, others are not

[xmxanuel]

This seems true and we currently don't support the meta-transactions for governance.

I don't see this as a problem.

If we change it in Managed it would also have implications on DripsHub which is not a ERC2771Context.

[CodeSandwich]

[dispute validity] The governance API should be as simple and self-contained as possible. If 3rd party contracts like Caller turn out to be buggy or malicious, governance should be able to pause and upgrade without any interferences.

[c4-sponsor]

CodeSandwich marked the issue as sponsor disputed

[GalloDaSballo]

I believe the Sponsor side to be valid in terms of disputing a vulnerability

I think the finding is valid as a Non-Critical Refactoring

R

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-c

[c4-judge]

GalloDaSballo marked the issue as grade-b