bunker.finance contest - samruna's results

Minoor issues, code improvements:

1. Flawed logic maybe? Code: https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L112 Description: In below loop, the require(balanceOf()).. check if done after the amount is added. But there is nothing after this line, which means this check is useless. Not sure if the check meant to be done before adding amount to totalAmount. for (uint256 i; i < length; ++i) { if (!is1155) { require(amounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s."); } totalAmount += amounts[i]; require(balanceOf(msg.sender, tokenIds[i]) >= amounts[i], "CNFT: Not enough NFTs to redeem"); }

Mitigation Check the logic one more time.

2. Flawed logic: Code:https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/ERC1155Enumerable.sol#L44

Description: In above function, the if(from == to) check is done after super..() is called. If the condition fails, then execution is halted. Is there a reason the check if not first line in funtion

Mitigation: Check logic.

3. Code refactoring Code:https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CErc20.sol#L177-189

Description: bool success; assembly { switch returndatasize() case 0 { // This is a non-standard ERC-20 success := not(0) // set success to true } case 32 { // This is a compliant ERC-20 returndatacopy(0, 0, 32) success := mload(0) // Set success = returndata of external call } default { // This is an excessively non-compliant ERC-20, revert. revert(0, 0) } }

Above code is commong for dotransferIn and dotransferOut functions. It can be extracted out to reduce contract size. Infact both functions can be combined to reduce size further.

Mitigation: Change logic to,

bool success = isTransferSuccessful();

function isTransferSuccessful() returns bool(success) { bool success; assembly { switch returndatasize() case 0 { // This is a non-standard ERC-20 success := not(0) // set success to true } case 32 { // This is a compliant ERC-20 returndatacopy(0, 0, 32) success := mload(0) // Set success = returndata of external call } default { // This is an excessively non-compliant ERC-20, revert. revert(0, 0) } } }

4. Unused function Code: https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CEther.sol#L158 Description: The doTransferIn() function has internal scope and has no implementation. Also not called from within the contract. Mitigation: Consider removing the unused function.

5. Unused variables: Code: https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L452-453 Description: In above functions, parameters liquidator and borrower are not used. Mitigation: Removed unused parameters.

1. Use of custome error better than revert() Code: https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L40

Description: Instead of doin require(a==b,"..."), make use of revert with custom error. With Solidity 0.8 and above the revert function is better in terms of gas fee usage. It'll revert the transaction and refund any unused gas fees.

Mitigation: Replace require(numSold < maxDaSupply, 'Auction sold out'); with If (tokenIds.length != amounts.length) error CNFT_AMountLengthMismatch();