Platform: Code4rena
Start Date: 26/08/2021
Pot Size: $200,000 USDC
Total HM: 17
Participants: 11
Period: 14 days
Judge: ghoulsol
Total Solo HM: 12
Id: 23
League: ETH
Rank: 7/11
Findings: 5
Award: $5,666.24
🌟 Selected for report: 3
🚀 Solo Findings: 0
467.1508 NOTE - $467.15
1401.4525 USDC - $1,401.45
JMukesh
Use of transfer might render ETH impossible to withdraw becuase after istanbul hardfork , there is increases in the gas cost of the SLOAD operation and therefore breaks some existing smart contracts.Those contracts will break because their fallback functions used to consume less than 2300 gas, and they’ll now consume more, since 2300 the amount of gas a contract’s fallback function receives if it’s called via Solidity’s transfer() or send() methods. Any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.
https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
https://blog.openzeppelin.com/opyn-gamma-protocol-audit/
manual review
use call() instead of transfer() to send eth
#0 - jeffywu
2021-09-11T17:51:32Z
This should be 0 (Non critical). ETH transfers still work as of today. This is a best practice recommendation. In the case that there is a hard fork that breaks ETH transfers the protocol can be upgraded to support the use of call()
#1 - ghoul-sol
2021-09-15T22:24:38Z
I aline with warden on this one. .transfer will not work if the account is a smart contract. Keeping as is.
136.2212 NOTE - $136.22
408.6635 USDC - $408.66
JMukesh
it lack the checking of the value that it is fresh or not, if data is not fresh it can affect exchange rate.
these following condition should be met to ensure that data is fresh
require(updateTime != 0, "Incomplete round"); require(answeredInRound >= roundId, "Stale price");
manual review
require(updateTime != 0, "Incomplete round"); require(answeredInRound >= roundId, "Stale price");
add above condition to ensure that , given data is fresh or not
#0 - jeffywu
2021-09-11T17:49:20Z
Duplicate of #92
155.7169 NOTE - $155.72
467.1508 USDC - $467.15
JMukesh
different parameter are being set in Approval event in transferFrom()
function transferFrom( address from, address to, uint256 amount ) external override returns (bool) { (bool success, uint256 newAllowance) = proxy.nTokenTransferFrom(currencyId, msg.sender, from, to, amount);
// Emit transfer events here so they come from the correct contract emit Transfer(from, to, amount);
// here first parameter should be owner and second should be spender // as mentioned in ntokenErc20.sol that is : // event Approval(address indexed owner, // address indexed spender, uint256 amount);
emit Approval(msg.sender, from, newAllowance); return success; }
This error may negatively impact
off-chain tools that are monitoring critical transfer events of the token.
manual review
#0 - jeffywu
2021-09-11T17:53:58Z
Duplicate #55, dispute the categorization. This should be Low Risk.
#1 - ghoul-sol
2021-09-15T22:18:08Z
External services might be affected but it's not clear how significant it would be. Most of the time events are not critical. Making this low risk.
155.7169 NOTE - $155.72
467.1508 USDC - $467.15
JMukesh
Due to unchecked return value from isContract() , if input value is EOA then all function's and contracts{PauseRouter, CompoundToNotionalV2} that depend on notionalProxy will become inaccessible
manual review
check the return value from the Iscontract to prevent this type of situation
#0 - jeffywu
2021-09-11T17:47:26Z
Duplicate of #44.
This should be 1 Low Risk. Whether or not it is an EOA there is no guarantee that whatever contract is supplied would be the correct one. The contract is ultimately upgradeable so in the worst case this could be fixed.
#1 - ghoul-sol
2021-09-15T22:51:41Z
Agree with low risk.
🌟 Selected for report: JMukesh
346.0377 NOTE - $346.04
1038.113 USDC - $1,038.11
JMukesh
function migrateBorrowFromCompound( address cTokenBorrow, uint256 cTokenRepayAmount, uint16[] memory notionalV2CollateralIds, uint256[] memory notionalV2CollateralAmounts, BalanceActionWithTrades[] calldata borrowAction ) ;
if the array length of notionalV2CollateralId , notionalV2CollateralAmounts and borrowAction is not equal it can lead to an error
manual review
check the input array length
155.7169 NOTE - $155.72
467.1508 USDC - $467.15
JMukesh
Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately
for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert
https://raw.githubusercontent.com/trailofbits/publications/master/reviews/hermez.pdf
manual reveiw
use a two-step procedure for all non-recoverable critical operations to prevent
irrecoverable mistakes.
#0 - jeffywu
2021-09-11T15:10:32Z
Duplicate #94