code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/escrows/GovTokenEscrow.sol#L19 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/escrows/SimpleERC20Escrow.sol#L16

Vulnerability details

Impact

The market currently assumes that all escrow implementations comply with the IEscrow interface. This is currently not the case, which can lead to the deposit functionality being bricked.

Proof of Concept

If the callOnDepositCallback function is set to true during construction of a market, the deposit function will attempt to call onDeposit on the escrow:

function deposit(address user, uint amount) public {
    IEscrow escrow = getEscrow(user);
    collateral.transferFrom(msg.sender, address(escrow), amount);
    if(callOnDepositCallback) {
        escrow.onDeposit();
    }
    emit Deposit(user, amount);
}

Currently, only INVEscrow implements that function, while SimpleERC20Escrow and GovTokenEscrow do not. If they were to be used on a market with said callOnDepositCallback set to true the deposit function would always revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure that all escrow contracts inherit from IEscrow to have compile time safety and get immediate feedback on a contract being non-compliant. For the contracts that do not need a certain function, such as onDeploy in this case, leave the function body empty.

c4-judge commented 2 years ago

0xean marked the issue as duplicate

Simon-Busch commented 1 year ago

Issue marked as satisfactory as requested by 0xean

c4-judge commented 1 year ago

Simon-Busch marked the issue as duplicate of #379

code-423n4 / 2022-10-inverse-findings