Inverse Finance contest - djxploit's results

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L531

Vulnerability details

Impact

In repay() users can repay their debt.

function repay(address user, uint amount) public {
        uint debt = debts[user];
        require(debt >= amount, "Insufficient debt");
        debts[user] -= amount;
        totalDebt -= amount;
        dbr.onRepay(user, amount);
        dola.transferFrom(msg.sender, address(this), amount);
        emit Repay(user, msg.sender, amount);
    }

There is a require condition, that checks if the amount provided, is greater than the debt of the user. If it is, then the function reverts. This is where the vulnerability arises.

repay function can be frontrun by an attacker. Say an attacker pay a small amount of debt for the victim user, by frontrunning his repay transaction. Now when the victim's transaction gets executed, the require condition will fail, as the amount of debt is less than the amount of DOLA provided. Hence the attacker can repeat the process to DOS the victim from calling the repay function.

Proof of Concept

1. Victim calls repay() function to pay his debt of 500 DOLA , by providing the amount as 500
2. Now attacker saw this transaction on mempool
3. Attacker frontruns the transaction, by calling repay() with amount provided as 1 DOLA
4. Attacker's transaction get's executed first due to frontrunning, which reduces the debt of the victim user to 499 DOLA
5. Now when the victim's transaction get's executed, the debt of victim has reduced to 499 DOLA, and the amount to repay provided was 500 DOLA. Now as debt is less than the amount provided, so the require function will fail, and the victim's transaction will revert. This will prevent the victim from calling repay function

Hence an attacker can DOS the repay function for the victim user

Tools Used

Manual review

Recommended Mitigation Steps

Implement DOS protection

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L82 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L116

Vulnerability details

Impact

The latestAnswer function does not allow viewPrice and getPrice to validate the output of the Chainlink oracle query. As a result, it is possible for off-chain orders to use stale results. latestRoundData is able to ensure the round is complete and has returned a valid/expected price by validating additional round data. This is documented here.

Proof of Concept

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L116

            uint price = feeds[token].feed.latestAnswer();

Tools Used

Manual review

Recommended Mitigation Steps

Consider using Chainlink's latestRoundData function instead of latestAnswer to validate the output correctly

1) State variable can be packed

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L53

2) Add unchecked keyword

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L362 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L379 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L124 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L111 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L534 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L287

3) Cache storage variables used consecutively to save gas

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L110-L111 - totalDueTokensAccrued https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L123-L124 , https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L136-L137 - balances[user] https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L123-L124, https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L136-L137 - dueTokensAccrued lastUpdated - https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L286-L287 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L246 - escrows[user] liquidationFeeBps - https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L605-L606 collateralFactorBps - https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L359-L360, https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L376-L377 fixedPrices - https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L79, https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L113 pendingOperator - https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L67-L68

4) Use calldata instead of memory

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L32-L33

5) require should be at start to save gas

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L195 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L303

6) Combine multiple mappings with same key to struct

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L25-L27 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L57-L59 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L19-L20 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L23-L28

7) Don’t emit storage variable

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L74 - operator https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L70

8) Splitting require() statements that use && saves gas (even a single &&)

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L173 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L184 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L195 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L448