Platform: Code4rena
Start Date: 04/01/2023
Pot Size: $60,500 USDC
Total HM: 15
Participants: 105
Period: 5 days
Judge: gzeon
Total Solo HM: 1
Id: 200
League: ETH
Rank: 81/105
Findings: 1
Award: $36.50
š Selected for report: 0
š Solo Findings: 0
š Selected for report: 0xSmartContract
Also found by: 0x1f8b, 0xAgro, 0xdeadbeef0x, 0xhacksmithh, 2997ms, Atarpara, Bnke0x0, Diana, HE1M, IllIllI, Josiah, Kalzak, Lirios, MalfurionWhitehat, MyFDsYours, Raiders, RaymondFam, Rolezn, SaharDevep, Sathish9098, Udsen, Viktor_Cortess, adriro, ast3ros, betweenETHlines, btk, chaduke, chrisdior4, cryptostellar5, csanuragjain, giovannidisiena, gz627, hl_, horsefacts, joestakey, juancito, ladboy233, lukris02, nadin, oyc_109, pauliax, peanuts, prady, sorrynotsorry, zaskoh
36.5015 USDC - $36.50
| Issue | Contexts | |
|---|---|---|
| LOW-1 | Change the order of emit | 1 |
| LOW-2 | Avoid to create a sender with address 0 | 1 |
| LOW-3 | Initilize() function can be called by anybody | 1 |
| LOW-4 | Lack the validation that the divisor should not be 0. | 1 |
| Issue | Contexts | |
|---|---|---|
| NC-1 | Solidity versions should be consistent | 1 |
| NC-2 | Change some functions into modifiers | 4 |
| NC-3 | Some enum values are never used | 1 |
| NC-4 | Open TODOs | 2 |
| NC-5 | Add the validation of start address | 1 |
| NC-6 | Rename the function of getDeposit | 1 |
It should be build the IEntryPoint first, then emit the event. This is to prevent from issues when building IEntryPoint failed.
Proof Of Concept
Recommended Mitigation Steps
Change it to:
entryPoint = IEntryPoint(payable(_newEntryPoint)); emit EntryPointChanged(address(_entryPoint), _newEntryPoint);
In createSender function, if we finally failed to create a sender, we return an address 0. And we validate outside when invoking this function, this behaviour is very dangerous and doesn't follow "minimum exposure" principle. We'd better to handle this situation inside the function itself.
Recommended Mitigation Steps
Revert directly at the end of this function.
Anyone has the access of init function and can set the owner and entryPointAddress.
initializer doesn't cover the ownership issue. It can only make sure the function is only initialized once. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol
Proof Of Concept https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L166-L176
Recommended Mitigation Steps
Add the modifier of mixedAuth
If b is 0, the transaction will be reverted because there is no validation that b != 0.
As it is a very basic function in math.sol, so we need to add a message to tell the customers why the transaction being reverted. Proof Of Concept https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/libs/Math.sol#L45-L47
Recommended Mitigation Steps
Add require(b!=0, "divisor should not be 0");
Most of solidity versions in the files are pragma solidity ^0.8.12;Other files are pragma solidity 0.8.12, it's better to make them consisten.
Proof Of Concept
pragma solidity ^0.8.12;
pragma solidity 0.8.12;
Recommended Mitigation Steps
Make the versions consistent
There are some functions which only do the validations. They are better to be modifier instead of functions
Proof Of Concept
Recommended Mitigation Steps
Change them into modifiers
It should be build the IEntryPoint first, then emit the event.
In the enum Rounding, the values of Up and Zero are never used. So this could be changed into a flag, like "isUp", this is already enough because the default of our calculation is to round down. There is no need to keep a value of down.
Proof Of Concept
ā enum Rounding { ā Down, // Toward negative infinity ā Up, // Toward infinity ā Zero // Toward zero ā }
Recommended Mitigation Steps Making these enums as one boolean flag is already enough.
An open TODO is present. It is recommended to avoid open TODOs as they may indicate programming errors that still need to be fixed.
Proof Of Concept
Recommended Mitigation Steps Resolve these TODOs
In getModulesPaginated function, We didn't validate if the start address is 0 or not. Also we didn't check if this address is in modules. When start is 0x0, we treat it as a normal input and we'd better to add a require to validate start address.
Recommended Mitigation Steps
Add a require to validate start address.
The name "getDeposit" is very confusing, it sounds like withdrawDeposit. Actually this function is to get the balance of deposit. So we can name it "getDepositBalance"
Proof Of Concept
Recommended Mitigation Steps Name it "getDepositBalance"
#0 - c4-judge
2023-01-22T15:34:18Z
gzeon-c4 marked the issue as grade-b
#1 - c4-sponsor
2023-02-09T12:35:03Z
livingrockrises marked the issue as sponsor confirmed