code-423n4 / 2023-01-biconomy-findings

7 stars 9 forks source link

Incorrect Uint Type Might Result In Wrong(Or Loss) Stake Value #452

Closed code423n4 closed 1 year ago

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/StakeManager.sol#L59-L73

Vulnerability details

Impact

With the help of this bug , the stake value stored in the DepositInfo struct can be incorrect .

Proof of Concept

1.) Alice wants to stake using addStake function here https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L59-L73 2.) Alice provides a msg.value , where msg.value > type(uint112).max (msg.value is provided in wei , so this gets more practical as 10^18wei = 1 ether). 3.) Now here , https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L63 stake is calculated and because msg.value > type(uint112).max (or (info.stake + msg.value) > type(uint112).max) , stake variable holds a value larger than what a uint112 can hold(does not over flow because this value is > uint256). 4.) At https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L69 , we cast the stake variable into a uint112 and as this value was more than a uint112 this will give an incorrect result or even a loss of stake value.

Tools Used

Manual Analysis

Recommended Mitigation Steps:

Use this line instead uint112 stake = info.stake + msg.value; So that it reverts when a value larger than type(uint112).max is provided.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid