Processes refinance operations may call malicious code by re-created refinancer contract

[code423n4]

Lines of code

https://github.com/maple-labs/loan/blob/main/contracts/MapleLoanInternals.sol#L258-L261

Vulnerability details

Impact

When an attacker (borrower) proposes a new term, the attacker can let a lender accept the malicious term which the lender doesn't expect.

It uses delegatecall in _acceptNewTerms of MapleLoanInternals.sol. Though a lender can manually check refinancer contract before calling acceptNewTerms, the attacker (borrower) can still re-create a malicious contract on same address before the lender is calling acceptNewTerms, and trigger malicious code by delegatecall in _acceptNewTerms.

Proof of Concept

In summary, an attacker can use CREATE2 to re-create a new malicious contract on same address. Here is CREATE2 exploit example: https://x9453.github.io/2020/01/04/Balsn-CTF-2019-Creativity/

1. An attacker (borrower) first deploy a refinancer contract with normal refinance actions to cheat lenders. The refinancer have malicious constructor which can be hidden in inherited contracts.
2. The attacker call proposeNewTerms, specifying a refinancer contract, and monitor acceptNewTerms in Mempool.
3. When the attacker monitored a lender calls acceptNewTerms, then quickly pack these transactions:
   1. Destroy refinancer contract by calling selfdestruct
   2. Use CREATE2 to re-deploy a new refinancer contract with malicious code on same address
   3. The lender calls acceptNewTerms
4. Then a lender will execute malicious code of new refinancer contract.

Tools Used

ethers.js

Recommended Mitigation Steps

Also check refinancer contract bytecodes in _getRefinanceCommitment:

    function _getRefinanceCommitment(address refinancer_, uint256 deadline_, bytes[] calldata calls_) internal pure returns (bytes32 refinanceCommitment_) {
        return keccak256(abi.encode(refinancer_, deadline_, calls_, at(refinancer)));
    }

    function at(address _addr) public view returns (bytes memory o_code) {
        assembly {
            // retrieve the size of the code, this needs assembly
            let size := extcodesize(_addr)
            // allocate output byte array - this could also be done without assembly
            // by using o_code = new bytes(size)
            o_code := mload(0x40)
            // new "memory end" including padding
            mstore(0x40, add(o_code, and(add(add(size, 0x20), 0x1f), not(0x1f))))
            // store length in memory
            mstore(o_code, size)
            // actually retrieve the code, this needs assembly
            extcodecopy(_addr, add(o_code, 0x20), 0, size)
        }
    }

[lucas-manuel]

Refinancer contracts are vetted by the smart contracts team. Any custom refinancer that is used will be able to have devastating consequences on the Loan as it performs custom delegatecalls. For this reason the assumption is made that Borrowers and Lenders will only use audited immutable Refinancer contracts that are deployed by the Maple Labs smart contracts team, and if not, they must be diligent enough to audit themselves or must accept any consequences.

This is not a valid issue.

[dmvt]

The report is valid. Given the potential danger involved, you should whitelist the allowable Refinancer contracts to protect your lenders. Issue stands.

[lucas-manuel]

We're providing smart contracts team validated refinancers to the application for the lenders and borrowers to use. If they choose to use externally developed refinancers (something we want to support as it will allow for highly customizable logic between lenders and borrowers which is a feature not a bug), they must audit themselves.

If there is a selfdestruct contained in the contract, it can be immediately assumed to be malicious because of the exploit outlined above. Same goes for proxied refinancers and stateful refinancers.

I do not agree with this being a High Risk issue due to its very high degree of difficulty. The borrower would have to develop a smart contract with this exploit, submit it outside of the application, and convince the Pool Delegate to accept the refinancer address also outside of the application without an audit.

I think that this is an interesting finding and am not discounting it, but I highly disagree with severity.

[dmvt]

I see your point. This exploit has external requirements and so should be medium, but I do think this represents a significant attack vector should a lender act accidentally against their own best interests.