code-423n4 / 2023-04-frankencoin-findings

5 stars 4 forks source link

Off by one error on `calculateProceeds` function leads to lock 2 shares instead of 1 #900

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L293

Vulnerability details

Impact

As described in comments that there should always be atleast 1 share but it use less then sign which makes it atleast 2 shares

Proof of Concept

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L293 In below code snipet as the comment describe there is always atleast one share but in require statement it adds shares with ONE_DEC18 constant which is equal to 1 share. Example: So if totalShares are 2 and I want to redeem 1 it will revert as 1 + 1 < 2condition will fail inside require.

require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one share 

Tools Used

Manual review

Recommended Mitigation Steps

Use less then or equal to sign

-- require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one share 
++ require(shares + ONE_DEC18 <= totalShares, "too many shares"); // make sure there is always at least one share 
c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as low quality report

0xA5DF commented 1 year ago

This is actually off by one wei rather than 1 wad, QA

c4-judge commented 1 year ago

hansfriese changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

hansfriese marked the issue as grade-b