Platform: Code4rena
Start Date: 25/10/2022
Pot Size: $50,000 USDC
Total HM: 18
Participants: 127
Period: 5 days
Judge: 0xean
Total Solo HM: 9
Id: 175
League: ETH
Rank: 29/127
Findings: 2
Award: $343.35
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: neumo
Also found by: BClabs, ladboy233, minhtrng, rvierdiiev
342.9734 USDC - $342.97
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
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.
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.
Manual Review
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.
#0 - c4-judge
2022-11-05T22:44:27Z
0xean marked the issue as duplicate
#1 - Simon-Busch
2022-12-05T15:31:29Z
Issue marked as satisfactory as requested by 0xean
#2 - c4-judge
2022-12-07T08:23:39Z
Simon-Busch marked the issue as duplicate of #379
🌟 Selected for report: rbserver
Also found by: 0x1f8b, 0xNazgul, 0xc0ffEE, 8olidity, Aymen0909, Chom, Franfran, Jeiwan, Jujic, Lambda, M4TZ1P, Olivierdem, Rolezn, Ruhum, TomJ, Wawrdog, __141345__, bin2chen, c7e7eff, carlitox477, catchup, cccz, codexploder, cuteboiz, d3e4, dipp, djxploit, eierina, elprofesor, hansfriese, horsefacts, idkwhatimdoing, imare, immeas, joestakey, ladboy233, leosathya, martin, minhtrng, pashov, peanuts, pedroais, rokinot, rvierdiiev, saneryee, sorrynotsorry, tonisives
0.385 USDC - $0.38
https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L6 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L82 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L116
The chainlink data is currently requested through an deprecated endpoint. Also, there are no checks to the validity of the data. Stale data could lead to users borrowing more than they should be allowed to, or users being liquidated even though they have enough collateral.
The Oracle
currently uses the latestAnswer
function of a Chainlink feed to retrieve current prices:
function viewPrice(address token, uint collateralFactorBps) external view returns (uint) { ... uint price = feeds[token].feed.latestAnswer(); ... } function getPrice(address token, uint collateralFactorBps) external returns (uint) { ... uint price = feeds[token].feed.latestAnswer(); ... }
This function is deprecated according to Chainlinks API reference for price feeds and offers no information on the latest time of update.
Manual Review
Use latestRoundData
instead (refer to Chainlink docs linked above). This function also returns the time of update, which can be used to check for staleness of data, which should be handled (for example by emitting events or having a pause mechanism for extended staleness).
#0 - neumoxx
2022-10-31T08:39:00Z
Duplicate of #601
#1 - c4-judge
2022-11-05T17:48:47Z
0xean marked the issue as duplicate
#2 - Simon-Busch
2022-12-05T15:29:31Z
Issue marked as satisfactory as requested by 0xean
#3 - c4-judge
2022-12-07T08:14:13Z
Simon-Busch marked the issue as duplicate of #584