Inverse Finance contest - Dinesh11G's results

Unsafe ERC20 Operation(s)

Impact

Issue Information: ERC20 operations can be unsafe due to different implementations and vulnerabilities in the standard.

It is therefore recommended to always either use OpenZeppelin's SafeERC20 library or at least to wrap each operation in a require statement.

To circumvent ERC20's approve functions race-condition vulnerability use OpenZeppelin's SafeERC20 library's safe{Increase|Decrease}Allowance functions.

In case the vulnerability is of no danger for your implementation, provide enough documentation explaining the reasonings.

Example 🤦 Bad:

IERC20(token).transferFrom(msg.sender, address(this), amount); 🚀 Good (using OpenZeppelin's SafeERC20):

import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";

// ...

IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

Findings:

2022-10-inverse/src/Fed.sol::135 => dola.transfer(gov, profit);
2022-10-inverse/src/Market.sol::205 => dola.transfer(msg.sender, amount);
2022-10-inverse/src/Market.sol::280 => collateral.transferFrom(msg.sender, address(escrow), amount);
2022-10-inverse/src/Market.sol::399 => dola.transfer(to, amount);
2022-10-inverse/src/Market.sol::537 => dola.transferFrom(msg.sender, address(this), amount);
2022-10-inverse/src/Market.sol::570 => dola.transfer(msg.sender, replenisherReward);
2022-10-inverse/src/Market.sol::602 => dola.transferFrom(msg.sender, address(this), repaidDebt);
2022-10-inverse/src/escrows/GovTokenEscrow.sol::45 => token.transfer(recipient, amount);
2022-10-inverse/src/escrows/INVEscrow.sol::50 => _token.approve(address(xINV), type(uint).max);
2022-10-inverse/src/escrows/INVEscrow.sol::63 => token.transfer(recipient, amount);
2022-10-inverse/src/escrows/SimpleERC20Escrow.sol::38 => token.transfer(recipient, amount);
2022-10-inverse/src/test/mocks/BorrowContract.sol::12 => WETH9(weth_).approve(address(market), type(uint).max);

Tools used

Manual

==============================================================================================

Unspecific Compiler Version Pragma

Impact

Issue Information: Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

Example 🤦 Bad:

pragma solidity ^0.8.0; 🚀 Good:

pragma solidity 0.8.4;

Findings:

2022-10-inverse/src/BorrowController.sol::2 => pragma solidity ^0.8.13;
2022-10-inverse/src/DBR.sol::2 => pragma solidity ^0.8.13;
2022-10-inverse/src/Fed.sol::2 => pragma solidity ^0.8.13;
2022-10-inverse/src/Market.sol::2 => pragma solidity ^0.8.13;
2022-10-inverse/src/Oracle.sol::2 => pragma solidity ^0.8.13;
2022-10-inverse/src/escrows/GovTokenEscrow.sol::2 => pragma solidity ^0.8.13;
2022-10-inverse/src/escrows/INVEscrow.sol::2 => pragma solidity ^0.8.13;
2022-10-inverse/src/escrows/SimpleERC20Escrow.sol::2 => pragma solidity ^0.8.13;
2022-10-inverse/src/test/BorrowController.t.sol::2 => pragma solidity ^0.8.13;

Tools used

Manual

==============================================================================================

Title:

Authorization through tx.origin

Description:

tx.origin is a global variable in Solidity which returns the address of the account that sent the transaction. Using the variable for authorization could make a contract vulnerable if an authorized account calls into a malicious contract. A call could be made to the vulnerable contract that passes the authorization check since tx.origin returns the original sender of the transaction which in this case is the authorized account.

Contract: BorrowController Function name: borrowAllowed(address,address,uint256) PC address: 876 Estimated Gas Usage: 924 - 1019

Impact:

Use of tx.origin as a part of authorization control. The tx.origin environment variable has been found to influence a control flow decision. Note that using tx.origin as a security control might cause a situation where a user inadvertently authorizes a smart contract to perform an action on their behalf. It is recommended to use msg.sender instead.

In file: BorrowController.sol:47

if(msgSender == tx.origin) return true

Initial State:

Account: [CREATOR], balance: 0x1, nonce:0, storage:{} Account: [ATTACKER], balance: 0x0, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 value: 0x0 Caller: [ATTACKER], function: borrowAllowed(address,address,uint256), txdata: 0xda3d454c000000000000000000000000000000000000000000408000020080400100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000, value: 0x0

Tools Used:

Manual

==============================================================================================

Lines are Too Long

Usually, lines in source code are limited to 80 characters. Today’s screens are much larger so it’s reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

There are 4 instances of this issue:

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

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/escrows/INVEscrow.sol#L25

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

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

==============================================================================================

commented code should be resolved

Lines of commented

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/escrows/GovTokenEscrow.sol#L56-L60 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/escrows/SimpleERC20Escrow.sol#L49-L53

Uncomment because the Escrow contract handles deposit callbacks

Tools used

Done Manually

==============================================================================================

External Call To User-Supplied Address

Contract: GovTokenEscrow Function name: initialize(address,address) PC address: 916

Impact

A call to a user-supplied address is executed. An external message call to an address specified by the caller is executed. Note that the callee account might contain arbitrary code and could re-enter any function within this contract. Reentering the contract in an intermediate state may lead to unexpected behaviour. Make sure that no state modifications are executed after this call and/or reentrancy guards are in place.

In file: GovTokenEscrow.sol

_token.delegate(_token.delegates(_beneficiary))

Initial State:

Account: [CREATOR], balance: 0x2, nonce:0, storage:{} Account: [ATTACKER], balance: 0x0, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [SOMEGUY], function: initialize(address,address), txdata: 0x485cc955000000000000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeef0000000000000000000000000000000000000000000000000000000000000000, value: 0x0

Tools used

Manual test

==============================================================================================

Multiple Calls in a Single Transaction

Contract: GovTokenEscrow Function name: initialize(address,address) PC address: 916

Impact

Multiple calls are executed in the same transaction. This call is executed following another call within the same transaction. It is possible that the call never gets executed if a prior call fails permanently. This might be caused intentionally by a malicious callee. If possible, refactor the code such that each transaction only executes one external call or make sure that all callees can be trusted (i.e. they’re part of your own codebase).

In file: GovTokenEscrow.sol

_token.delegate(_token.delegates(_beneficiary))

---

Initial State:

Account: [CREATOR], balance: 0x218, nonce:0, storage:{} Account: [ATTACKER], balance: 0x0, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [CREATOR], function: initialize(address,address), txdata: 0x485cc95500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000, value: 0x0

Tools used

Manual test

==============================================================================================

External Call To User-Supplied Address

Contract: GovTokenEscrow Function name: delegate(address) PC address: 1151

Impact

A call to a user-supplied address is executed. An external message call to an address specified by the caller is executed. Note that the callee account might contain arbitrary code and could re-enter any function within this contract. Reentering the contract in an intermediate state may lead to unexpected behaviour. Make sure that no state modifications are executed after this call and/or reentrancy guards are in place.

In file: GovTokenEscrow.sol

token.delegate(delegatee)

---

Initial State:

Account: [CREATOR], balance: 0xc000040007bc7d, nonce:0, storage:{} Account: [ATTACKER], balance: 0x161074001, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [ATTACKER], function: initialize(address,address), txdata: 0x485cc955000000000000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeef000000000000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeef, value: 0x0 Caller: [ATTACKER], function: delegate(address), txdata: 0x5c19a95c0000000000000000000000001e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e, value: 0x0

Tools used

Manual test

==============================================================================================

External Call To User-Supplied Address

Contract: GovTokenEscrow Function name: pay(address,uint256) PC address: 1623

Impact

A call to a user-supplied address is executed. An external message call to an address specified by the caller is executed. Note that the callee account might contain arbitrary code and could re-enter any function within this contract. Reentering the contract in an intermediate state may lead to unexpected behaviour. Make sure that no state modifications are executed after this call and/or reentrancy guards are in place.

In file: GovTokenEscrow.sol

token.transfer(recipient, amount)

---

Initial State:

Account: [CREATOR], balance: 0x4000000007b2b5, nonce:0, storage:{} Account: [ATTACKER], balance: 0x820080, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [ATTACKER], function: initialize(address,address), txdata: 0x485cc955000000000000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeef0000000000000000000000000000000000000000000000000000000000000000, value: 0x0 Caller: [ATTACKER], function: pay(address,uint256), txdata: 0xc4076876000000000000000000000000010101010101010101010101010101021e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e, value: 0x0

Tools used

Manual test

==============================================================================================

External Call To User-Supplied Address

Contract: SimpleERC20Escrow Function name: pay(address,uint256) PC address: 962

Impact

A call to a user-supplied address is executed. An external message call to an address specified by the caller is executed. Note that the callee account might contain arbitrary code and could re-enter any function within this contract. Reentering the contract in an intermediate state may lead to unexpected behaviour. Make sure that no state modifications are executed after this call and/or reentrancy guards are in place. In file: SimpleERC20Escrow.sol:38

token.transfer(recipient, amount)

Initial State:

Account: [CREATOR], balance: 0x9fdf9, nonce:0, storage:{} Account: [ATTACKER], balance: 0x1000000200, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , value: 0x0 Caller: [ATTACKER], function: initialize(address,address), txdata: 0x485cc955000000000000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeef0000000000000000000000000000000000000000000000000000000000000000, value: 0x0 Caller: [ATTACKER], function: pay(address,uint256), txdata: 0xc407687600000000000000000000000001010101410101010101010101010102484848484848484848484848484848484848484848484848484848484848484848484848, value: 0x0

Tools Used

Manual

Use != 0 instead of > 0 for Unsigned Integer Comparison

Impact

Issue Information: When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Example 🤦 Bad:

// a being of type unsigned integer require(a > 0, "!a > 0"); 🚀 Good:

// a being of type unsigned integer require(a != 0, "!a > 0");

Findings:

2022-10-inverse/src/DBR.sol::63 => require(newReplenishmentPriceBps_ > 0, "replenishment price must be over 0");
2022-10-inverse/src/DBR.sol::328 => require(deficit > 0, "No deficit");
2022-10-inverse/src/Fed.sol::133 => if(profit > 0) {
2022-10-inverse/src/Market.sol::75 => require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps < 10000, "Invalid liquidation incentive");
2022-10-inverse/src/Market.sol::162 => require(_liquidationFactorBps > 0 && _liquidationFactorBps <= 10000, "Invalid liquidation factor");
2022-10-inverse/src/Market.sol::173 => require(_replenishmentIncentiveBps > 0 && _replenishmentIncentiveBps < 10000, "Invalid replenishment incentive");
2022-10-inverse/src/Market.sol::184 => require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps + liquidationFeeBps < 10000, "Invalid liquidation incentive");
2022-10-inverse/src/Market.sol::195 => require(_liquidationFeeBps > 0 && _liquidationFeeBps + liquidationIncentiveBps < 10000, "Invalid liquidation fee");
2022-10-inverse/src/Market.sol::561 => require(deficit > 0, "No DBR deficit");
2022-10-inverse/src/Market.sol::592 => require(repaidDebt > 0, "Must repay positive debt");
2022-10-inverse/src/Market.sol::605 => if(liquidationFeeBps > 0) {
2022-10-inverse/src/Oracle.sol::79 => if(fixedPrices[token] > 0) return fixedPrices[token];
2022-10-inverse/src/Oracle.sol::83 => require(price > 0, "Invalid feed price");
2022-10-inverse/src/Oracle.sol::96 => uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
2022-10-inverse/src/Oracle.sol::97 => if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
2022-10-inverse/src/Oracle.sol::113 => if(fixedPrices[token] > 0) return fixedPrices[token];
2022-10-inverse/src/Oracle.sol::117 => require(price > 0, "Invalid feed price");
2022-10-inverse/src/Oracle.sol::135 => uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
2022-10-inverse/src/Oracle.sol::136 => if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
2022-10-inverse/src/escrows/INVEscrow.sol::81 => if(invBalance > 0) {

Tools used

Manual

---

Use immutable for OpenZeppelin AccessControl's Roles Declarations

Impact

Issue Information: Only valid for solidity versions <0.6.12 ⚡️

Access roles marked as constant results in computing the keccak256 operation each time the variable is used because assigned operations for constant variables are re-evaluated every time.

Changing the variables to immutable results in computing the hash only once on deployment, leading to gas savings.

Example 🤦 Bad:

bytes32 public constant GOVERNOR_ROLE = keccak256("GOVERNOR_ROLE"); 🚀 Good:

bytes32 public immutable GOVERNOR_ROLE = keccak256("GOVERNOR_ROLE");

Findings:

2022-10-inverse/src/DBR.sol::227 => keccak256(
2022-10-inverse/src/DBR.sol::231 => keccak256(
2022-10-inverse/src/DBR.sol::233 => keccak256(
2022-10-inverse/src/DBR.sol::268 => keccak256(
2022-10-inverse/src/DBR.sol::270 => keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
2022-10-inverse/src/DBR.sol::271 => keccak256(bytes(name)),
2022-10-inverse/src/DBR.sol::272 => keccak256("1"),
2022-10-inverse/src/Market.sol::103 => keccak256(
2022-10-inverse/src/Market.sol::105 => keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
2022-10-inverse/src/Market.sol::106 => keccak256(bytes("DBR MARKET")),
2022-10-inverse/src/Market.sol::107 => keccak256("1"),
2022-10-inverse/src/Market.sol::303 => mstore(add(ptr, 0x6c), keccak256(ptr, 0x37))
2022-10-inverse/src/Market.sol::304 => predicted := keccak256(add(ptr, 0x37), 0x55)
2022-10-inverse/src/Market.sol::426 => keccak256(
2022-10-inverse/src/Market.sol::430 => keccak256(
2022-10-inverse/src/Market.sol::432 => keccak256(
2022-10-inverse/src/Market.sol::490 => keccak256(
2022-10-inverse/src/Market.sol::494 => keccak256(
2022-10-inverse/src/Market.sol::496 => keccak256(

Tools used

Manual

---

Long Revert Strings

Impact

Issue Information: Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

If the contract(s) in scope allow using Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

Example 🤦 Bad:

require(condition, "UniswapV3: The reentrancy guard. A transaction cannot re-enter the pool mid-swap"); 🚀 Good (with shorter string):

// TODO: Provide link to a reference of error codes require(condition, "LOK");

Findings:

2022-10-inverse/src/DBR.sol::63 => require(newReplenishmentPriceBps_ > 0, "replenishment price must be over 0");
2022-10-inverse/src/DBR.sol::234 => "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
2022-10-inverse/src/DBR.sol::270 => keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
2022-10-inverse/src/DBR.sol::326 => require(markets[msg.sender], "Only markets can call onForceReplenish");
2022-10-inverse/src/Market.sol::76 => require(_replenishmentIncentiveBps < 10000, "Replenishment incentive must be less than 100%");
2022-10-inverse/src/Market.sol::105 => keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
2022-10-inverse/src/Market.sol::214 => require(msg.sender == pauseGuardian || msg.sender == gov, "Only pause guardian or governance can pause");
2022-10-inverse/src/Market.sol::433 => "BorrowOnBehalf(address caller,address from,uint256 amount,uint256 nonce,uint256 deadline)"
2022-10-inverse/src/Market.sol::497 => "WithdrawOnBehalf(address caller,address from,uint256 amount,uint256 nonce,uint256 deadline)"

Tools used

Manual

---

Improper liquidationFactorBps was set

Impact:

It leads to an increase in the Gas value when <= 10000 is used in https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L162

Remidation

It is fixed by using < 10000 which lowers the Gas fee

Findings

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

Tools:

Manual