Platform: Code4rena
Start Date: 14/06/2022
Pot Size: $100,000 USDC
Total HM: 26
Participants: 59
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 9
Id: 133
League: ETH
Rank: 19/59
Findings: 8
Award: $1,576.32
🌟 Selected for report: 0
🚀 Solo Findings: 0
1207.2233 CANTO - $194.97
194.9666 USDC - $194.97
Attackers can drain wrapped Manifest token and all ethers that the contract owns.
Lending Market (Compound Fork) contains WETH.sol which has a vulnerability.
https://github.com/code-423n4/2022-06-newblockchain#weth-80-sloc
Any person can call approve function and set any owner and spender.
function approve(address owner, address spender) external returns(bool) { _approve(owner, spender, _balanceOf[owner]); return true; }
Here are steps of the issue:
approve(address owner, address spender) function with owner = victim address and spender = attacker address in its arguments.transferFrom function with src = victim addres, dst = attacker address and wad = token amount that victim owns in its argumentswithdraw function and withdraw ethers.Static code analysis Remix
Either remove problematic approve(address owner, address spender) function or add more checks inside approve(address owner, address spender) function
#0 - nivasan1
2022-06-21T22:20:06Z
duplicate of issue #19
🌟 Selected for report: Soosh
Also found by: 0x1f8b, Ruhum, TerrierLover, WatchPug, cccz, csanuragjain, hake, p4st13r4, zzzitron
782.2807 CANTO - $126.34
126.3383 USDC - $126.34
Anybody can override the existing proposal
AddProposal function does not have any checks to prevent the override. Malicious users can set any values at the existing proposal and modify them.
function AddProposal(uint propId, string memory title, string memory desc, address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas) public { Proposal memory newProp = Proposal(propId, title, desc, targets, values, signatures, calldatas); proposals[propId] = newProp; }
Static code analysis
Add some rules to prevent the override such as followings.
proposals contains propId already, it does not allow the override happensProposal struct, and rewrite the codebase so only the owner can update the existing proposal#0 - nivasan1
2022-06-24T01:23:17Z
duplicate of #26
🌟 Selected for report: p4st13r4
Also found by: Ruhum, TerrierLover, WatchPug, hansfriese, zzzitron
1987.1989 CANTO - $320.93
320.9326 USDC - $320.93
Total supply does not return the correct value.
totalSupply() function returns _balanceOf[address(this)] which does not reflect the correct total supply.
function totalSupply() public view returns (uint) { return _balanceOf[address(this)]; }
_balanceOf of this contract will only be updated when somebody calls either transfer or transferFrom with dst= the address of contract
Static code analysis Remix
totalSupply() function should implement other ways to correctly return the total supply. This is an example of the workaround:
function totalSupply() public view returns (uint) { return this.balance; }
#0 - ecmendenhall
2022-06-21T22:28:30Z
#1 - nivasan1
2022-06-23T21:07:42Z
duplicate of #191
🌟 Selected for report: joestakey
Also found by: 0x1f8b, 0x29A, 0x52, 0xDjango, 0xNazgul, 0xf15ers, 0xmint, Bronicle, Dravee, Funen, JMukesh, Limbooo, MadWookie, Picodes, Ruhum, TerrierLover, TomJ, Tutturu, WatchPug, Waze, _Adam, asutorufos, c3phas, catchup, cccz, codexploder, cryptphi, csanuragjain, defsec, fatherOfBlocks, gzeon, hake, hansfriese, hyh, ignacio, k, nxrblsrpr, oyc_109, robee, sach1r0, saian, simon135, technicallyty, zzzitron
75.3722 USDC - $75.37
687.9945 CANTO - $111.11
As solidity document mentions here, it should be named with all capital letters with underscores separating words. However, following state variables do not follow this pattern.
According to the solidity doc, using assert should be avoided.
Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix.
Following codes use assert, but it should be changed into require or others.
Return variables are defined at following functions, but they are not necessary since the function uses return statement.
For example, getAmountOut function can be written like this:
function getAmountOut(uint amountIn, address tokenIn, address tokenOut) external view returns (uint, bool) {
supplyMarket(uint amount ) has extra space between amount and )
https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L464
function _safeTransfer(address token,address to,uint256 value) does not have enough space
https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Accountant/AccountantDelegator.sol#L54
This should be written like this: function _safeTransfer(address token, address to, uint256 value)
faialure should be failure
https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L463window is 0, infinite loop hapens at sample functionWhen window is 0, i will not be updated and for loop will be in infinite loop
https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L227
getAmountsOut function can return uint array which contains 0 valuefor (uint i = 0; i < routes.length; i++) { address pair = pairFor(routes[i].from, routes[i].to, routes[i].stable); if (IBaseV1Factory(factory).isPair(pair)) { amounts[i+1] = IBaseV1Pair(pair).getAmountOut(amounts[i], routes[i].from); } }
When IBaseV1Factory(factory).isPair(pair) is not true, amounts[i+1] will not be updated, and remain 0. Therefore, getAmountsOut function can return uint array which contains 0 value. If this is not expected, it should handle this case gracefully.
#0 - GalloDaSballo
2022-08-03T23:41:28Z
R
##Â [QA-2] Avoid using assert L
R
NC
NC
NC as it's self DOS
Disagree as the 0 value is pretty graceful to me
I really like the formatting of this report, short and sweet
1L 2R 3NC
🌟 Selected for report: _Adam
Also found by: 0v3rf10w, 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, 0xmint, Chom, Dravee, Fitraldys, Funen, JC, Limbooo, MadWookie, Picodes, Ruhum, TerrierLover, TomJ, Tomio, Waze, ak1, c3phas, catchup, defsec, fatherOfBlocks, gzeon, hake, hansfriese, joestakey, k, oyc_109, rfa, robee, sach1r0, saian, simon135, ynnad
41.2642 USDC - $41.26
396.9199 CANTO - $64.10
!= 0 instead of > 0 on uint variablesuint variables will never be lower than 0. Therefore, > 0 and != 0 have same meanings. Using != 0 can reduce the gas deployment cost, so it is worth using != 0 wherever possible.
Here are list of the gas improvements by using != 0.
The default value of uint varibles are 0. Therefore, there is no need to set 0 on uint variables. Not setting 0 on uint variables can reduce the deployment gas cost.
Here are list of the gas improvements.
Following variables or operations can be wrapped by unchecked to reduce the gas cost.
[1] i++ or ++i used in the for loop when the end condition is uint or constant
For example, this for loop can be written like this since numTokens is uint256, and it is certain that ++i will not overflow:
for (uint i = 0; i < numTokens;) { setCompSpeedInternal(cTokens[i], supplySpeeds[i], borrowSpeeds[i]); unchecked { ++i; } }
[2] y > y_prev assures that y - y_prev in if statement, and y_prev - y in else statement will not underflow
unchecked { if (y > y_prev) { if (y - y_prev <= 1) { return y; } } else { if (y_prev - y <= 1) { return y; } } }
[3] if (msg.value > amountCANTO) assures that msg.value - amountCANTO will not underflow
unchecked { if (msg.value > amountCANTO) _safeTransferCANTO(msg.sender, msg.value - amountCANTO); }
[4] require(b <= a, ...) assures that a-b will not underflow
function sub256(uint256 a, uint256 b) internal pure returns (uint) { require(b <= a, "subtraction underflow"); unchecked { return a - b; } }
[5] if (amountToSubtract > currentAccrual) assures that amountToSubtract - currentAccrual will not underflow
if (amountToSubtract > currentAccrual) { // Amount of COMP the user owes the protocol unchecked { uint accountReceivable = amountToSubtract - currentAccrual; // Underflow safe since amountToSubtract > currentAccrual }
Using custom errors can reduce the gas cost.
== true is not needed when checking the boolean value is trueFor example, this part can be written like this:
if (marketToJoin.accountMembership[borrower]) { // already joined return Error.NO_ERROR; }
At permit function, it sets DOMAIN_SEPARATOR every time the function is called. DOMAIN_SEPARATOR can be set beforehand, and it can avoid setting DOMAIN_SEPARATOR every time.
name and symbolThe difference of is statement and else statment are as follows:
if (_stable) { name = string(abi.encodePacked("StableV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol())); symbol = string(abi.encodePacked("sAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol())); } else { name = string(abi.encodePacked("VolatileV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol())); symbol = string(abi.encodePacked("vAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol())); }
This part can be written as follows:
string memory namePrefix; string memory symbolPrefix; if (_stable) { namePrefix = "StableV1 AMM - "; symbolPrefix = "sAMM-"; } else { namePrefix = "VolatileV1 AMM - "; symbolPrefix = "vAMM-"; } name = string(abi.encodePacked(namePrefix, erc20(_token0).symbol(), "/", erc20(_token1).symbol())); symbol = string(abi.encodePacked(symbolPrefix, erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
block.number directly instead of calling getBlockNumber() functionCalling block.number directly instead of getBlockNumber() function can reduce the gas cost.
#0 - GalloDaSballo
2022-08-04T00:44:12Z
150 on the DOMAIN_SEPARATOR, rest is less than 500 gas