Biconomy - Smart Contract Wallet contest - MyFDsYours's results

Summary

[L-01] Lack of two steps procedure for new ownership
[NC-01] RETURNDATASIZE opcode is called 3 times 
[NC-02] Repeated function getNonce() and nonce()
[NC-03] Missing docstrings
[NC-04] Unnamed return parameters

[L-01] Lack of two steps procedure for new ownership

In SmartAccount.sol L109

The owner of the smart account have critical rights, (like transferring funds, executing multiples transactions and so on), 
then it very useful to implement a two steps confirmation that gives to the owner the right to make a mistake (set an incorrect 
owner address for example) without necessarily having a great consequence.  

Recommandation

[NC-01] RETURNDATASIZE opcode is called 3 times L25

In Proxy.sol in the fallback function, RETURNDATASIZE opcode is called 3 times. 

Recommendation:

Use intermediate variables inside the assembly in Proxy.sol contract, to avoid executing opcodes
multiple times. 

Instead of this:

returndatacopy(0, 0, returndatasize())
switch success
case 0 { revert(0, returndatasize()) }
default { return(0, returndatasize()) }

Use cached variable:

let rds := returndatasize()
returndatacopy(0, 0, rds)
switch success
case 0 { revert(0, rds) }
default { return(0, rds) }

Also present in ModuleManager L93

[NC-02] Repeated function getNonce() and nonce()

In SmartAccount.sol L97

These two [ getNonce() and nonce() ] have to return the number of transaction by giving the batchId; 

Having two functions that return the same thing is not necessary and can be confusing.

Recommandation

Remove the nouce() function and keep getNounce() function that is more explecit and documented 

[NC-03] Missing docstrings

Throughout the codebase there are several parts that do not have docstrings. Some examples are:

L449 in the SmartAccount contract

L455 in the SmartAccount contract

L460 in the SmartAccount contract

L465 in the SmartAccount contract

L494 in the SmartAccount contract

When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

[NC-04] Unnamed return parameters

Consider adding and using named return parameters to improve explicitness and readability.

In SmartAccount.sol L140

Recommandation