code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/BasePaymaster.sol#L75-L77 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/paymasters/BasePaymaster.sol#L75-L77

Vulnerability details

Impact

The Ethereum could get locked inside the BasePayMaster.sol contract if the external call to the EntryPoint contract reverts due to require statement failure. In such a scenario there is no withdraw function inside the BasePayMaster.sol contract to withdraw the Eth.

Proof of Concept

The addStake is an external payable function defined in the BasePayMaster.sol contract. If an external function calls this function with msg.value then the msg.value is sent to the EntryPoint.sol contract since entryPoint.addStake function is called with the input variable unstakeDelaySec.

function addStake(uint32 unstakeDelaySec) external payable onlyOwner {
    entryPoint.addStake{value : msg.value}(unstakeDelaySec);
}

But inside the addStake function in the StakeManager.sol contract there is require function to check whether unstakeDelaySec > 0 and _unstakeDelaySec >= info.unstakeDelaySec.

function addStake(uint32 _unstakeDelaySec) public payable {
    DepositInfo storage info = deposits[msg.sender];
    require(_unstakeDelaySec > 0, "must specify unstake delay");
    require(_unstakeDelaySec >= info.unstakeDelaySec, "cannot decrease unstake time");

If any of the above require statement fail then the transaction will revert and the ethereum will be sent back to the BasePayMaster.sol and not to the original msg.sender. But there is no withdraw function inside the BasePayMaster.sol contract to withdraw the Ether and hence the Ether will be locked inside the BasePayMaster.sol contract.

Tools Used

Manual Review, VSCode

Recommended Mitigation Steps

Return a Boolean success value to the addStake function of the BasePayMaster.sol contract based on result of the addStake external call function execution of the StakeManager contract.

If the external call transaction is a failure the addStake function of the BasePayMaster.sol can revert the transaction and can transfer back the ether to the msg.sender. Hence the ether will not be locked inside the BasePayMaster.sol contract.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor disputed

livingrockrises commented 1 year ago

68 is not duplicate of this

livingrockrises commented 1 year ago

will be asking in infinitism community