Closed sherlock-admin4 closed 7 months ago
@Hash01011122 Would it be possible to provide a reason as to why this submission was invalidated? I couldn't find the reason mentioned in any of the duplicates as well.
Due to hardcoded PCT_BASE any token which does not have 18 decimals can never be deposited in the protocol, or in worst case scenario can drain all of depositors balance (if they have enough USDC)
Given that USDC is one of the tokens to be deposited, this breaks one of the core functionality of the protocol and should be a valid finding.
For ref:
Escalate
This looks like a valid issue.
Escalate
This looks like a valid issue.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Just adding context since this issue has been escalated while #176 hasn't.
This has the same root cause as #176
This issue leads to failure to deposit(), which is a DOS. While #176 leads to failure in claim(), equivalent to locking/losing users' funds. The severity of both issues can be different, given that both have significantly different attack vectors and effects.
I leave the rest to the judges to decide.
In blast USDC has 18 decimals. Blast USDC
In blast USDC has 18 decimals. Blast USDC
Given that the Blast USDC has only 14 holders and a total supply of 19.5K USDC, it doesn't seem like the actual USDC that is supposed to be used.
Moreover, the name is USDC+
and not USDC.
Furthermore, it is highly unusual for L2s to change the decimal places of a stablecoin; they almost always adhere to the original decimals, 6 in the case of USDC.
This consistency ensures compatibility and interoperability across different layers and solutions within the Ethereum ecosystem.
As of now I don't think the original USDC has yet been deployed on blast, even as per documentation, bridged stablecoins like USDC, USDT or DAI are converted to USDB as of now.
However, this does not mean that USDC will not be supported in future, and since the comments from sponsors confirms that USDC will be supported and is planned, this means it can not be invalidated as per the Future Issue criteria and should be considered valid.
@Hash01011122, even if the USDB token is used and an amount with 18 decimals is passed, this amount is being multiplied by 1e18 and is then used for a transfer. This means that the contract will try to transfer 100e36 amount for 100 tokens, for example. This is why I escalated.
@Hash01011122 Thanks for your comment
Deposited amount is already in token decimals. https://github.com/sherlock-audit/2024-03-zap-protocol/blob/main/zap-contracts-labs/contracts/TokenSale.sol#L224 is upscaled to token decimal * 1e18
,further which is compared with sum i.e sum should have same units of token decimal * 1e18
. It clears the first point that amount deposited is in token decimals eg. 150USDC = 1501e6 or 150 1e18 depending on the decimal of token
After scaling up , it tries to transfer tokens without scaling down which is the core issue #51 . Most of the duped issue of #87 try to point out this issue
maxAllocationOfUser
has 18 decimals which is incorrect (it has token decimal * 1e18
decimal's precision )hence invalid i guessAlso Take a look #191 which is valid and different from this issue
PS: Correct me If i'm wrong
Thanks for pointing it @Tri-pathi I think #51, #84 and #94 are indeed valid issue.
@Hash01011122 Thanks for your comment
- Deposited amount is already in token decimals. https://github.com/sherlock-audit/2024-03-zap-protocol/blob/main/zap-contracts-labs/contracts/TokenSale.sol#L224 is upscaled to
token decimal * 1e18
,further which is compared with sum i.e sum should have same units oftoken decimal * 1e18
. It clears the first point that amount deposited is in token decimals eg. 150USDC = 1501e6 or 150 1e18 depending on the decimal of token
What is the assumption for the fact that input _amount
is already in token decimals? If it is true, then indeed, #87 and other dupes are invalid, while #51 and #84 are valid.
@Hash01011122, can we get confirmation from the sponsors regarding this? If the input is already in token decimals and we are assuming that Blast USDC/USDB will have 18 decimals, then I don't see a reason to multiply it with PCT_BASE for higher precision, as 18 decimals should be more than enough for any kind of precision! Based on this, my assumption was that input might not be considering token decimals.
Moreover, #94 explicitly states, USDC transferred is incorrect, and PCT_BASE (10**18) is used as the unit of USDC. However, the decimals of USDC are 6, so the amount of USDC transferred is incorrect.
However, it also assumes that the input is in token decimals, so in a way it has both the assumptions of #87 (i.e., incorrect decimal system for USDC) and #51 (i.e., input is with token decimals). So I am not sure which one to club it with.
What is the assumption for the fact that input _amount is already in token decimals?
At line 224 it computes maxAllocationOfUser
which returns maxAllocation
function calculateMaxAllocation(address _sender) public returns (uint256) {
uint256 userMaxAllc = _maxTierAllc(_sender);
if (userMaxAllc > maxAllocation) {
return userMaxAllc;
} else {
return maxAllocation;
}
}
and we can see crystal clear comment at 39 that maxAllocation is in token decimals
Now maxAllocationOfuser is in token decimals * PCT_BASE
which is compared with amount
. That shows that input amount should be in token decimals which is upscaled by PCT_BASE for comparison with maxAllocationOfuser
And thanks for pointing out #94 is also incorrectly duped. Concluding my points with this and leaving it to lead judge's decision
PS: correct me if i'm wrong
and we can see crystal clear comment at 39 that maxAllocation is in token decimals
I see what you are saying and the logic seems valid because of the comment that maxAllocation
already includes token decimals. So based on that uint256 maxAllocationOfUser = (calculateMaxAllocation(_sender)) *PCT_BASE
should mean maxAllocationOfUser
has either 24 or 36 decimals (based on whether USDC has 6 or 18 decimals) and will fail regardless of the deposit amount or the decimals of USDC.
However, I assumed that maybe that comment was a typo, given that there are already so many typos in the codebase. This assumption was made because functions like processPrivate()
, calculateMaxAllocatio
, etc., all work perfectly fine if the input does not have token decimals.
Let's consider the following scenario (I've added comments on lines where calculations need to be explained):
maxAllocation
does not include decimals has a typo in the comment, and has a value set to 100 instead of 100 * 10^6_processPrivate()
has _amount
, which does not include token decimals, for example, 200 instead of 100 * 10^6.calculateMaxAllocation
returns maxAllocation
as 100. uint256 maxAllocationOfUser = (calculateMaxAllocation(_sender)) * PCT_BASE; // 100 * 10^18
require(sum <= maxAllocationOfUser, "upto max allocation");
uint256 taxFreeAllcOfUser = 0; // hardcode zero - all pools have ax
uint256 userTaxAmount;
if (sum > taxFreeAllcOfUser) {
uint256 userTxRate = userTaxRate(sum, _sender);
if (s.amount < taxFreeAllcOfUser) { // this condition will never be hit as taxFreeAllcOfUser = 0
userTaxAmount =
((sum - taxFreeAllcOfUser) * userTxRate) /
POINT_BASE;
} else {
userTaxAmount = (amount * userTxRate) / POINT_BASE; // (200 * 10 ^18 * userTxRate) / POINT_BASE
}
}
In the above case, if USDC has 18 decimals on blast, the codebase has no issues, as the deposit will not show any weird behavior.
Given that there are no tests for this file, we can not be sure whether _amount
contains decimals or not.
However, if it does include decimals, it becomes obvious that the codebase will always fail and the entire function will be broken, whereas if _amount
does not contain decimals, it will work as long as the assumption that USDC
has 18 decimals on Blast is TRUE.
BUT, if USDC has 6 decimals on Blast (as it should with all other L2s), then this deposit()
will certainly fail.
Note:
Hence, I think we need to confirm with the sponsors what their intention is with the input _amount
and whether they intend to use USDC in future or not (as far as the comments on discord and in codebase are concerned, it certainly seems USDC is planned).
@Hash01011122 @Tri-pathi let me know what you think?
As far as I know, token input will be with decimals of the token, but it would be better if @niketansainiantier can confirm this one
@Hash01011122
There is no test and from the comments and docs it is pretty much understandable that input amount will be in token decimals. As SR, our evaluation and analysis should prioritize documentation, comments, and code for any assumptions made
@Hash01011122 any comments from @niketansainiantier or any other sponsor over this issue?
This issue described is not specific to USDC but to the contract breaking with any token that's not 18 decimals.
If true, this would be a valid Medium as the following rule applies
Breaks core contract functionality, rendering the contract useless or leading to loss of funds.
This issue described is not specific to USDC but to the contract breaking with any token that's not 18 decimals.
If true, this would be a valid Medium as the following rule applies
Breaks core contract functionality, rendering the contract useless or leading to loss of funds.
agreed, and the same will be true for #176 which leads to failure to in claim()
and eventually lead to locking of funds of the user.
@Hash01011122 can you list the duplicates of this issue?
@Evert0x This is definitely not a valid issue. The deposit
function is not meant to be adjusted for decimals. Furthermore, all computation in the contract is meant to be done in terms of USDB (which has 18 decimals). Computation in terms of the token itself is not even done.
@Evert0x Valid issues are #51, #84, and #94 where issue states unwanted upscaling of 10e18 of PCT_BASE. Rest issues as @detectiveking123 has pointed out are invalid. On second thoughts entire family of this issue can be invalid if the input of token decimals isn't in 10e18 format
@Hash01011122 As you stated, the input is not meant to be in 10e18 format. The entire family of issues is invalid.
@Hash01011122 As you stated, the input is not meant to be in 10e18 format. The entire family of issues is invalid.
This issue will be invalid if the current or any future ERC20 token to be used by the contract always contains 18 decimals.
However, one of the major reasons this issue was raised is that it is repeatedly mentioned as USDC in Discord, moreover, the token variable is written as USDC in the codebase.
So, I had to assume that the token was supposed to be USDC, and if it is USDC, then it will definitely have 6 decimals instead of 18 decimals on Blast as per the reasoning provided here
What is the assumption for the fact that input _amount is already in token decimals?
At line 224 it computes
maxAllocationOfUser
which returns maxAllocationfunction calculateMaxAllocation(address _sender) public returns (uint256) { uint256 userMaxAllc = _maxTierAllc(_sender); if (userMaxAllc > maxAllocation) { return userMaxAllc; } else { return maxAllocation; } }
and we can see crystal clear comment at 39 that maxAllocation is in token decimals
Now maxAllocationOfuser is in
token decimals * PCT_BASE
which is compared withamount
. That shows that input amount should be in token decimals which is upscaled by PCT_BASE for comparison withmaxAllocationOfuser
And thanks for pointing out #94 is also incorrectly duped. Concluding my points with this and leaving it to lead judge's decision
PS: correct me if I'm wrong
Due to comments and code, it is crystal clear that input value will be in token decimals.
which protocol gives inputs value in plain integers ?
Even for a sec we assume that inputs are plain integers , we didn't have any prior information/validation about this during the contest [Even now neither from sponsors and nor from contest page/docs/comments/codebase ]. It is very uncommon behavior so it should be added in the contest page .
Where did lead judge stated this ?
@Hash01011122 As you stated, the input is not meant to be in 10e18 format. The entire family of issues is invalid.
from what can i see, This was the last comment from The judge
As far as I know, token input will be with decimals of the token
I have explained why i think input tokens will be token decimals[code logic + comments] . So please share if you have any proof/comments to make sure that inputs are in plain integers other than just assumptions
and spam.
Received confirmation from the sponsors that the token input is in wei and they are going to remove PCT_BASE from their codebase. This makes #51, #72, #84 and #94 issue valid and rest can be invalidated @Evert0x. Also @Nilay27 please refrain to comment same logical argument you presented earlier, as USDC and USDB in blast is 18 decimals.
@Hash01011122, why is 72 excluded? It shows the exact same upscaling issue.
Received confirmation from the sponsors that the token input is in wei and they are going to remove PCT_BASE from their codebase. This makes #51, #72, #84 and #94 issue valid and rest can be invalidated @Evert0x. Also @Nilay27 please refrain to comment same logical argument you presented earlier, as USDC and USDB in blast is 18 decimals.
If the sponsors have confirmed the input with decimals, then I have no objection to marking this issue invalid. It only partially represents the problem that any token breaking 18 decimals will fail regardless of whether the input is with decimals or not, and it's up to Sherlock to consider this scenario valid/invalid.
I only gave the previous logic because the token in the link is not USDC but USDC+ and has a dubious number of holders and total supply.
Thanks for taking the effort to clarify the situation with sponsors. :)
As the readme / codebase / docs / discord message do not capture the intended behavior of this function, I believe it's fair to base the judgment on the current protocol opinion.
For that reason, planning to make 51, 72, 84, and 94 and valid Medium family
Breaks core contract functionality, rendering the contract useless or leading to loss of funds.
@Evert0x Could you kindly elaborate on which core contract functionality is broken?
@Evert0x
Agree with @deadrosesxyz
Please elaborate. If this issue is valid off of bad documentation, there are so many other issues in this contest that should be valid, just because the documentation is completely wrong.
@deadrosesxyz @detectiveking123 The following issues #51, #72, #84 and #94 are valid because those issues states unwanted upscaling of 10e18 which was confirmed by the dev and sponsors. If you guys still feel it's invalid please mention why it should be invalidated.
After taking another detailed look, I believe this issue should stay invalid.
First of all, the context of this function was unclear during the audit. Which doesn't create a strong foundation to validate this or related issues.
Second of all, the issues that are being proposed to become a family are not sufficient for Medium severity. The effect of the scaling is that users are not able to deposit fractional deposits (only rounded amounts).
I will reject escalation and keep issue as is.
@Evert0x
First of all, the context of this function was unclear during the audit. Which doesn't create a strong foundation to validate this or related issues
How the context of function was unclear? There is normal deposit function which doesn't work well.
The effect of the scaling is that users are not able to deposit fractional deposits (only rounded amounts).
Please go through the submissions before making comments like this . Effect of scaling is that users deposits the upscaled amounts
. Sponsors already confirmed that input amount is in wei i.e in token decimals .So why do you guys have second thoughts.
Even @Hash01011122 has repeatedly confirmed the validity of this submission from starting.
If any of this unclear to you then give a proper comment but after going through the submissions.
@Tri-pathi I've never made this comment Effect of scaling is that users deposits the upscaled amounts
What exactly is your counter argument against my actual comment?
The effect of the scaling is that users are not able to deposit fractional deposits (only rounded amounts).
@Evert0x
What exactly is your counter argument against my actual comment?
The effect of the scaling is that users are not able to deposit fractional deposits (only rounded amounts).
Taken From #84 : Here's a scenario provided for better uderstanding :
_amount
= 1
(in wei as confirmed by dev ) ._amount
gets scaled up to amount = 1 * 1e18 = 1e18 . Which means 1000 Billion tokens ! ( current overall circulation of USDC is ~31 Billion tokens ) . msg.sender
. Which will always fail .So deposit functionality will be broken forever and always revert. And is a core functionality of the protocol which gets broken .
Result: Invalid Has Duplicates
nilay27
high
Incorrect Precision of PCT_BASE in
TokenSale.sol
will cause failure indeposit
of USDCSummary
In
TokenSale.sol
the user deposit can happen via USDC, but the base multiplier for deposit amount isPCT_BASE=10 ** 18
. This will severy functionality breakdown because USDC only works with 6 decimals.Vulnerability Detail
In
TokenSale.sol
we have a const PCT_BASE defined as:In the
deposit
method, we have the following code:where
_processPrivate
is defined as:Impact
The deposit will most likely always FAIL with USDC as the actual amount will be
10 ** 12
intended deposit and most likely greater than the balence of User. This will Severly break down the functionality of TokenSale with USDC as deposits, which as confirmed by the Sponsor is a valid token to deposit.Code Snippet
https://github.com/sherlock-audit/2024-03-zap-protocol/blob/main/zap-contracts-labs/contracts/TokenSale.sol#L33
https://github.com/sherlock-audit/2024-03-zap-protocol/blob/main/zap-contracts-labs/contracts/TokenSale.sol#L182
https://github.com/sherlock-audit/2024-03-zap-protocol/blob/main/zap-contracts-labs/contracts/TokenSale.sol#L221-L248
POC
Below is a foundry test for simulating the
deposit
with amockUSDC
token with 6 decimals. I alsoinitialize
the constructor argument in theTokenSale
to accomodate for MockUSDC as it is currently hardcoded.Foundry Test Code
```solidity pragma solidity ^0.8.13; import {Test, console} from "forge-std/Test.sol"; import {TokenSale} from "../src/TokenSale.sol"; import {ITokenSale} from "../src/interfaces/ITokenSale.sol"; import {IAirdrops} from "../src/interfaces/IAirdrops.sol"; import {IERC20D} from "../src/interfaces/IERC20D.sol"; import {IStaking} from "../src/interfaces/IStaking.sol"; import {MockUSDC} from "./mocks/mockUSDC.sol"; import {Admin} from "../src/Admin.sol"; // import {Staking} from "../src/Staking.sol"; contract TokenSaleTest is Test { TokenSale public tokenSale; address public user1 = makeAddr('user1'); address public user2 = makeAddr('user2'); address public adminOwner = makeAddr('adminOwner'); address public operator = makeAddr('operator'); address public staking = makeAddr('staking'); MockUSDC public usdc; // 0xA9F81589Cc48Ff000166Bf03B3804A0d8Cec8114 // // Prepare the bytecode for MockUSDC // bytes mockUSDCBytecode = type(MockUSDC).creationCode; Admin public adminContract; bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00; bytes32 public constant OPERATOR = keccak256("OPERATOR"); // Staking staking = new Staking(); function setUp() public { ITokenSale.Params memory params; params.totalSupply = uint96(10**18); params.privateStart = uint32(block.timestamp); params.privateTokenPrice = uint96(10**18); params.privateEnd = uint32(block.timestamp + 100); tokenSale = new TokenSale(); usdc = new MockUSDC(); // setup the admin contract, owner, operator and set userKYC adminContract = new Admin(); adminContract.initialize(adminOwner); vm.startPrank(adminOwner); adminContract.addOperator(operator); vm.stopPrank(); // set kyc for user1 and user2 vm.startPrank(operator); address[] memory users = new address[](2); users[0] = user1; users[1] = user2; adminContract.setUserKYC(users); vm.stopPrank(); // setup the token sale contract tokenSale.initialize(params, staking, address(adminContract), 100*10**18, 0, true, 0, address(usdc)); console.log(usdc.balanceOf(user1)); usdc.mint(user1, 100*10**6); console.log(usdc.balanceOf(user1)); } function stakingMockCalls() public{ vm.mockCall( address(staking), abi.encodeWithSelector(IStaking.getUserState.selector, address(user1)), abi.encode(1,1,1,1) ); vm.mockCall( address(staking), abi.encodeWithSelector(IStaking.getAllocationOf.selector, address(user1)), abi.encode(10) ); // Mock for getAllocations function uint256 arg1 = 2; // Example argument 1 uint256 arg2 = 1; // Example argument 2 uint128 returnValue = 9; // Example return value vm.mockCall( address(staking), abi.encodeWithSelector(IStaking.getAllocations.selector, arg1,arg2), abi.encode(returnValue) ); } function test_deposit() public{ vm.startPrank(user1); // tokenSale.deposit(1); // user wants to depoist 1 usdc console.log("kyc", adminContract.isKYCDone(user1)); stakingMockCalls(); usdc.approve(address(tokenSale), type(uint256).max); tokenSale.deposit(1); // user wants to depoist 1 usdc vm.stopPrank(); } } ```Console Output
```solidity [128165] TokenSale::deposit(1) │ ├─ [631] Admin::isKYCDone(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) │ │ └─ ← true │ ├─ [2827] Admin::blacklist(TokenSale: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) │ │ └─ ← false │ ├─ [0] staking::getUserState(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) │ │ └─ ← 1, 1, 1, 1 │ ├─ [0] staking::getAllocationOf(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) │ │ └─ ← 10 │ ├─ [0] staking::getAllocations(2, 1) │ │ └─ ← 9 │ ├─ [3322] MockUSDC::transferFrom(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], TokenSale: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1000000000000000000 [1e18]) │ │ └─ ← revert: ERC20: transfer amount exceeds balance │ └─ ← revert: ERC20: transfer amount exceeds balance └─ ← revert: ERC20: transfer amount exceeds balance ```Tool used
Manual Review, Foundry
Recommendation
Instead of having a fixed value of
uint256 constant PCT_BASE = 10 ** 18
, have a mapping of acceptable Tokens that can be deposited into the contract