-
- L-01. If a winner is blacklisted on any of the tokens they can't receive their funds
- L-02. Owner can incorrectly pull funds from contests not yet expired
- L-03. Lack of checking the existence of the Proxy contract
- L-04. Signature missing nonce & expiration deadline
- L-05. Precision loss/Rounding to Zero in
_distribute() - L-06. Potential DOS due to Gas Exhaustion Due to Large Array Iteration in
_distributeFunction - L-07. Centralization Risk for trusted organizers
- L-08. DAI Tokens at Risk Due to Lack of address(0) Check in distribute
- L-09. Missing Events
- L-10. Using basis points for percentage is not precise enough for realistic use-cases
- L-11. Insufficient validation leads to locking up prize tokens forever
- L-12. Organizers are not incentivized to deploy and distribute to winners causing that winners may not to be rewarded for a long time and force the protocol owner to manage the distribution
- High: 1
- Medium: 3
- Low: 12
H-01. The same signature can be used in different distribution implementation causing that the caller who owns the signature, can distribute on unauthorized implementations
Submitted by pep7siup, carrotsmuggler, nmirchev8, alexfilippov314, Cosine, 0xbepresent, CMierez, dacian, sobieski, VanGrim, jonatascm, zach030, Madalad, Agkistrodon, ACai, toshii, 0x4non, alra, Silver Hawks, honeymewn, yixxas, golanger85, ubermensch, dipp, pontifex, serialcoder, maanas. Selected submission by: 0xbepresent.
The same signature can be used in different distribute implementations causing that the caller who owns the signature, to distribute on unauthorized implementations.
The ProxyFactory::setContest() function helps to configure a closeTime to specific organizer, contestId and implementation.
File: ProxyFactory.sol
105: function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
106: public
107: onlyOwner
...
...
113: bytes32 salt = _calculateSalt(organizer, contestId, implementation);
114: if (saltToCloseTime[salt] != 0) revert ProxyFactory__ContestIsAlreadyRegistered();
115: saltToCloseTime[salt] = closeTime;The caller who owns the signature, can distributes to winners using the deployProxyAndDistributeBySignature() function. The problem is that the hash in the code line (#159) does not consider the implementation parameter.
File: ProxyFactory.sol
152: function deployProxyAndDistributeBySignature(
153: address organizer,
154: bytes32 contestId,
155: address implementation,
156: bytes calldata signature,
157: bytes calldata data
158: ) public returns (address) {
159: bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(contestId, data)));
160: if (ECDSA.recover(digest, signature) != organizer) revert ProxyFactory__InvalidSignature();
161: bytes32 salt = _calculateSalt(organizer, contestId, implementation);
162: if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
163: if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
164: address proxy = _deployProxy(organizer, contestId, implementation);
165: _distribute(proxy, data);
166: return proxy;
167: }For some reason, there could be a different distribution implementation to the same contestId. Then the caller who owns the signature can distribute even if the organizer does not authorize a signature to the new implementation.
I created a test where the caller who owns a signature can distribute to new distribute implementation using the same signature. Test steps:
- Owner setContest using the implementation
address(distributor) - Organizer creates a signature.
- Caller distributes prizes using the signature.
- For some reason there is a new distributor implementation. The Owner set the new distributor for the same
contestId. - The caller can distribute prizes using the same signature created in the step 2 in different distributor implementation.
// test/integration/ProxyFactoryTest.t.sol:ProxyFactoryTest
// $ forge test --match-test "testSignatureCanBeUsedToNewImplementation" -vvv
//
function testSignatureCanBeUsedToNewImplementation() public {
address organizer = TEST_SIGNER;
bytes32 contestId = keccak256(abi.encode("Jason", "001"));
//
// 1. Owner setContest using address(distributor)
vm.startPrank(factoryAdmin);
proxyFactory.setContest(organizer, contestId, block.timestamp + 8 days, address(distributor));
vm.stopPrank();
bytes32 salt = keccak256(abi.encode(organizer, contestId, address(distributor)));
address proxyAddress = proxyFactory.getProxyAddress(salt, address(distributor));
vm.startPrank(sponsor);
MockERC20(jpycv2Address).transfer(proxyAddress, 10000 ether);
vm.stopPrank();
assertEq(MockERC20(jpycv2Address).balanceOf(proxyAddress), 10000 ether);
// before
assertEq(MockERC20(jpycv2Address).balanceOf(user1), 0 ether);
assertEq(MockERC20(jpycv2Address).balanceOf(stadiumAddress), 0 ether);
//
// 2. Organizer creates a signature
(bytes32 digest, bytes memory sendingData, bytes memory signature) = createSignatureByASigner(TEST_SIGNER_KEY);
assertEq(ECDSA.recover(digest, signature), TEST_SIGNER);
vm.warp(8.01 days);
//
// 3. Caller distributes prizes using the signature
proxyFactory.deployProxyAndDistributeBySignature(
TEST_SIGNER, contestId, address(distributor), signature, sendingData
);
// after
assertEq(MockERC20(jpycv2Address).balanceOf(user1), 9500 ether);
assertEq(MockERC20(jpycv2Address).balanceOf(stadiumAddress), 500 ether);
//
// 4. For some reason there is a new distributor implementation.
// The Owner set the new distributor for the same contestId
Distributor new_distributor = new Distributor(address(proxyFactory), stadiumAddress);
vm.startPrank(factoryAdmin);
proxyFactory.setContest(organizer, contestId, block.timestamp + 8 days, address(new_distributor));
vm.stopPrank();
bytes32 newDistributorSalt = keccak256(abi.encode(organizer, contestId, address(new_distributor)));
address proxyNewDistributorAddress = proxyFactory.getProxyAddress(newDistributorSalt, address(new_distributor));
vm.startPrank(sponsor);
MockERC20(jpycv2Address).transfer(proxyNewDistributorAddress, 10000 ether);
vm.stopPrank();
//
// 5. The caller can distribute prizes using the same signature in different distributor implementation
vm.warp(20 days);
proxyFactory.deployProxyAndDistributeBySignature(
TEST_SIGNER, contestId, address(new_distributor), signature, sendingData
);
}The caller who owns the signature, can distribute the prizes for a new distribution implementation using the same signature which was created for an old implementation.
The organizer must create a new signature if there is a new implementation for the same contestId. The authorized signature is for one distribution implementation not for the future distribution implementations.
Manual review
Include the distribution implementation in the signature hash.
function deployProxyAndDistributeBySignature(
address organizer,
bytes32 contestId,
address implementation,
bytes calldata signature,
bytes calldata data
) public returns (address) {
-- bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(contestId, data)));
++ bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(contestId, implementation, data)));M-01. The digest calculation in deployProxyAndDistributeBySignature does not follow EIP-712 specification
Submitted by nmirchev8, 0x6980, CMierez, maanas, T1MOH, 97Sabit, arnie, RugpullDetector, Silver Hawks, qpzm. Selected submission by: CMierez.
The calculation of the digest done in ProxyFactory.deployProxyAndDistributeBySignature() does not follow the EIP-712 specification. It is missing the function's corresponding typeHash, as well as the hashStruct calculation of the data signature parameter, which are both defined in the EIP.
Not following the EIP specification will end up in unexpected integration failures with EIP712-compliant wallets or tooling that perform the encoding in the appropriate way.
In ProxyFactory.deployProxyAndDistributeBySignature(), the digest is calculated as follows:
bytes32 digest = _hashTypedDataV4(
keccak256(
abi.encode(contestId, data)
)
);The EIP-712 specification defines the encoding of a message as:
"\x19\x01" ‖ domainSeparator ‖ hashStruct(message)
In the current implementation, "\x19\x01" and domainSeparator are correctly calculated and appended as per OpenZeppelin's _hashTypedDataV4() function, but hashStruct(message) is not respected.
The EIP defines that the hashStruct of a message is calculated from the hashing of the typeHash and the encoding of the data; and the former is currently missing in the digest calculation.
Additionally, the data parameter which is being included as part of the signature, is a bytes type, which the EIP defines as Dynamic. Dynamic types are encoded as the hash of the contents; and currently the data parameter is being encoded as-is.
The data being signed is not being encoded as per the EIP-712 specification, which will result in unexpected integration failures with EIP712-compliant wallets or tooling that perform the encoding in the appropriate way.
After looking at the tests, I would say this error was not caught since the tests themselves follow the same exact implementation for creating the data being signed. Usage of external libraries such as Ethers.js would have likely revealed this issue earlier.
Manual Review
- Define the
typeHash
bytes32 internal constant DEPLOY_AND_DISTRIBUTE_TYPEHASH = keccak256(
"DeployAndDistribute(bytes32 contestId,bytes data)"
);- Include it in the
digestcalculation
bytes32 digest = _hashTypedDataV4(
keccak256(
abi.encode(
DEPLOY_AND_DISTRIBUTE_TYPEHASH,
contestId,
...
)
)
);bytes32 digest = _hashTypedDataV4(
keccak256(
abi.encode(
DEPLOY_AND_DISTRIBUTE_TYPEHASH,
contestId,
keccak256(data)
)
)
);Submitted by pep7siup, dontonka, bronzepickaxe, imkapadia, DevABDee, thekmj, InAllHonesty, 33BYTEZZZ, Kose, Madalad, castleChain, Aamirusmani1552, Tripathi, 0x4non, crippie, Cosine, Silver Hawks, 0xhals, tsar, radeveth, dipp, MrjoryStewartBaxter, serialcoder. Selected submission by: pep7siup.
https://github.com/Cyfrin/2023-08-sparkn/tree/main/src/Distributor.sol#L164
The vulnerability relates to the immutability of STADIUM_ADDRESS. If this address is blacklisted by the token used for rewards, the system becomes unable to make transfers, leading to funds being stuck in the contract indefinitely.
- Owner calls
setContestwith the correctsalt. - The Organizer sends USDC as rewards to a pre-determined Proxy address.
STADIUM_ADDRESSis blacklisted by the USDC operator.- When the contest is closed, the Organizer calls
deployProxyAndDistributewith the registeredcontestIdandimplementationto deploy a proxy and distribute rewards. However, the call toDistributor._commissionTransferreverts at Line 164 due to the blacklisting. - USDC held at the Proxy contract becomes stuck forever.
// Findings are labeled with '<= FOUND'
// File: src/Distributor.sol
116: function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
117: ...
154: _commissionTransfer(erc20);// <= FOUND
155: ...
156: }
...
163: function _commissionTransfer(IERC20 token) internal {
164: token.safeTransfer(STADIUM_ADDRESS, token.balanceOf(address(this)));// <= FOUND: Blacklisted STADIUM_ADDRESS address cause fund stuck in the contract forever
165: }This vulnerability is marked as High severity because a blacklisted STADIUM_ADDRESS would lead to funds being locked in the Proxy address permanently. Funds are already held in the Proxy, and the Proxy's _implementation cannot be changed once deployed. Even the ProxyFactory.distributeByOwner() function cannot rescue the funds due to the revert.
Manual Review
It is recommended to allow STADIUM_ADDRESS to be updatable by a dedicated admin role to avoid token transfer blacklisting. Moreover, since STADIUM_ADDRESS is no longer immutable, storage collision should be taken into account.
Submitted by 0xch13fd357r0y3r, 0xdeth, sv , InAllHonesty, GoSoul22, 0xnevi, savi0ur, Bughunter101, SBSecurity, 0x3b, cats, nisedo, MrjoryStewartBaxter, KiteWeb3, Soliditors, DevABDee, Phantasmagoria, Breeje, y4y, VanGrim, 0xanmol, Madalad, FalconHoof, shikhar229169, TheSchnilch, 0xDetermination, ABA, Aamirusmani1552, zigtur, ke1caM, t0x1c, Stoicov, arnie, 0xMosh, SAAJ, AkiraKodo, owade, honeymewn, coolboymsk, Maroutis, 0xScourgedev, 0x11singh99. Selected submission by: Madalad.
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol#L116
The contest details state that 'If a contest is created and funded, there is no way to refund. All the funds belong to the persons who wants to help solve the problem, we call them "supporters".' (see More Context section). This is untrue, as the organizer is able to refund all of the contest funds.
In Distributor#_distribute, there is no input validation on the winners array. A malicious or compromised organizer can, with little effort, simply pass an array of length one containing a wallet address that they control as the winners parameter, and [10000] as the percentages parameter in order to receive 100% of the funds initially deposited to the contract. Due to the design of the protocol, they would have 7 days after the contest ends (the value of the EXPIRATION_TIME constant in the ProxyFactory contract) to perform this action without the owner being able to prevent it.
Malicious/Compromised organizer can refund 100% of the contest funds, stealing work from sponsors.
Manual review
Use a two step procedure for distributing funds:
- The organizer submits an array of winners and percentages to the
Proxycontract and they are cached using storage variables - The owner of
ProxyFactor(a trusted admin) checks the arrays to ensure the organizer is not distributing all of the money to themselves, and if satisfied, triggers the distribution of funds
This removes the risk of having to trust the organizer, and although it requires the trust of the admin, they were already a required trusted party and so the mitigation is beneficial overall. Also, this new system adds more truth to the statement from the contest details mentioned in the summary section of this report.
Submitted by ZedBlockchain, Giorgio, InAllHonesty, B353N, MrjoryStewartBaxter, pep7siup, carrotsmuggler, kodyvim, castleChain, deadrosesxyz, dontonka, Proxy, tsvetanovv, alymurtazamemon, bronzepickaxe, 0xyPhilic, oualidpro, kaliberpoziomka, 0x4ka5h, 0x3b, Arabadzhiev, Lalanda, crippie, Scoffield, 0xdraiakoo, Soliditors, thekmj, Stoicov, 33BYTEZZZ, 0xRizwan, Polaristow, VanGrim, TorpedopistolIxc41, Madalad, Kose, ohi0b, kamui, Tripathi, Maroutis, aslanbek, ke1caM, Bauer, Alhakista, Chinmay, trachev, arnie, RugpullDetector, tsar, 0xMosh, Cryptic Snake REACH, Cosine, Cosmic Bee, 0xsandy, 0xhals, smbv1923, sm4rty, ubermensch, golanger85, honeymewn, radeveth, 0xd3g3ns, Bauchibred, SanketKogekar, 0xScourgedev, 0xlucky. Selected submission by: Bauchibred.
Normally this would be a big issue since transfers are done in a loop to all winners i.e all winners wouldn't be able to get their tokens, but winners are chosen off chain and from the Q&A section of SparkN onboarding video we can see that after picking a set of winners they can later on be changed, that's the set of winners. This means that, reasonably, after an attempt to send the tokens to winners has been made and it reverts due to one or a few of the users being in the blacklist/blocklist of USDC/USDT, the set of winners can just be re-chosen without the blacklisted users, now whereas that helps other users from having their funds locked in the contract, this unfairly means that the blacklisted users would lose their earned tokens, since their share must be re-shared to other winners to cause this not to revert
if (totalPercentage != (10000 - COMMISSION_FEE)) {
revert Distributor__MismatchedPercentages();
}See summary
Additionally note that, the contest readMe's section has indicated that that general stablecoins would be used... specifically hinting USDC, DAI, USDT & JPYC,
Now important is to also keep in mind that https://github.com/d-xo/weird-erc20#tokens-with-blocklists shows that:
Some tokens (e.g. USDC, USDT) have a contract level admin controlled address blocklist. If an address is blocked, then transfers to and from that address are forbidden.
Two impacts, depending on how SparkN decides to sort this out, either:
-
All winners funds ends up stuck in contract if sparkN doesn't want to change the percentages of each winner by setting that of blacklisted users to zero and sharing their percentages back in the pool
-
Some users would have their funds unfairly given to other users
Manual Audit
Consider introducing a functionality that allows winners to specify what address they'd like to be paid, that way even a blocklisted account can specify a different address he/she owns, this case also doesn't really sort this as an attacker could just send any blacklisted address to re-grief the whole thing, so a pull over push method could be done to transfer rewards to winners
With this attack window in mind, if a pull method is going to be used then the _commisionTransfer() function needs to be refactored to only send the commision.
Submitted by meetm, klaus, t0x1c, kaliberpoziomka, aviggiano, alexfilippov314, 0x4ka5h, bowtiedvirus, Scoffield, Agkistrodon, maanas, castleChain, ohi0b, Arabadzhiev, igorline, NoamYakov, toshii, 0xMosh, RugpullDetector, Tricko, ryanjshaw, sonny2k, Silver Hawks, 0xhals, Rotcivegaf, ubermensch, serialcoder. Selected submission by: t0x1c.
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L195-L218
Owner can incorrectly pull funds from a closed contest which has not yet expired using distributeByOwner().
The distributeByOwner() function has 5 parameters: proxy, organizer, contestId, implementation, data. However, there is no 'linkage' between proxy and the remaining params.
In order to check if the contest has expired, it uses bytes32 salt = _calculateSalt(organizer, contestId, implementation);. There is no check if this is indeed the salt of the proxy address. Hence, the owner can by mistake call distributeByOwner() with an incorrect proxy address of a contest which is closed, but not yet expired and drain its funds incorrectly.
PoC: (run via forge test --mt test_OwnerCanIncorrectlyPullFundsFromContestsNotYetExpired -vv)
function test_OwnerCanIncorrectlyPullFundsFromContestsNotYetExpired() public {
// Imagine that 2 contests are started by the same organizer & sponsor. This is just for
// simplicity; the organizers/sponsors can be considered as different too for the contests in question.
vm.startPrank(factoryAdmin);
bytes32 randomId_1 = keccak256(abi.encode("Jason", "015")); // contest_1
bytes32 randomId_2 = keccak256(abi.encode("Watson", "016")); // contest_2
proxyFactory.setContest(organizer, randomId_1, block.timestamp + 8 days, address(distributor));
proxyFactory.setContest(organizer, randomId_2, block.timestamp + 10 days, address(distributor));
vm.stopPrank();
bytes32 salt_1 = keccak256(abi.encode(organizer, randomId_1, address(distributor)));
address proxyAddress_1 = proxyFactory.getProxyAddress(salt_1, address(distributor));
bytes32 salt_2 = keccak256(abi.encode(organizer, randomId_2, address(distributor)));
address proxyAddress_2 = proxyFactory.getProxyAddress(salt_2, address(distributor));
vm.startPrank(sponsor);
// sponsor funds both his contests
MockERC20(jpycv2Address).transfer(proxyAddress_1, 10000 ether);
MockERC20(jpycv2Address).transfer(proxyAddress_2, 500 ether);
vm.stopPrank();
// before
assertEq(MockERC20(jpycv2Address).balanceOf(user1), 0 ether, "user1 balance not zero");
assertEq(MockERC20(jpycv2Address).balanceOf(stadiumAddress), 0 ether, "STADIUM balance not zero");
assertEq(MockERC20(jpycv2Address).balanceOf(proxyAddress_1), 10000 ether, "proxy1 balance not 10000e18");
assertEq(MockERC20(jpycv2Address).balanceOf(proxyAddress_2), 500 ether, "proxy2 balance not 500e18");
bytes memory data = createData();
// 9 days later, organizer deploy and distribute -- for contest_1
vm.warp(9 days);
vm.prank(organizer);
proxyFactory.deployProxyAndDistribute(randomId_1, address(distributor), data);
// sponsor send token to proxy by mistake
vm.prank(sponsor);
MockERC20(jpycv2Address).transfer(proxyAddress_1, 11000 ether);
// 11 days later, organizer deploy and distribute -- for contest_2
vm.warp(11 days);
vm.prank(organizer);
proxyFactory.deployProxyAndDistribute(randomId_2, address(distributor), data);
// sponsor send token to proxy by mistake
vm.prank(sponsor);
MockERC20(jpycv2Address).transfer(proxyAddress_2, 600 ether);
// create data to send the token to admin
bytes memory dataToSendToAdmin = createDataToSendToAdmin();
// 16 days later from the start date, contest_1 has EXPIRED,
// but contest_2 is only CLOSED, not "EXPIRED".
// Hence, Owner should NOT be able to distribute rewards from funds reserved for contest_2.
vm.warp(16 days);
vm.prank(factoryAdmin);
// Owner provides `proxyAddress_2` by mistake, but remaining params are for `contest_1`
proxyFactory.distributeByOwner(proxyAddress_2, organizer, randomId_1, address(distributor), dataToSendToAdmin);
// above call should have reverted with "ProxyFactory__ContestIsNotExpired()"
// after
// STADIUM balance has now become (5% of 10000) + (5% of 500) + 600
assertEq(MockERC20(jpycv2Address).balanceOf(stadiumAddress), 1125 ether, "STADIUM balance not 1125e18");
assertEq(MockERC20(jpycv2Address).balanceOf(proxyAddress_1), 11000 ether, "proxy1 balance not 11000e18");
// contest_2 is fully drained
assertEq(MockERC20(jpycv2Address).balanceOf(proxyAddress_2), 0, "proxy2 balance not zero");
}The above is even more serious if Owner is trying to return the funds to sponsor1 using distributeByOwner(). Sponsor1 will get Sponsor2's funds (95% of funds, at the most).
OR
If the owner, upon a request from the sponsor, is trying to distribute contest_1's extra funds deposited by the sponsor as rewards to its winners. These winners would be completely different from the winners of contest_2, but funds from contest_2 will be redirected to "winner*1s".
Noteworthy is the fact that once any sponsor deposits extra funds by mistake later on *(after proxy has been deployed via deployProxyAndDistribute() or similar functions & the rewards have been distributed once)_ he can only take the help of the owner to send the funds to any specific address(es).
- Loss of funds as it can be drained by the owner by mistake from a not-yet-expired contest.
- Funds/Rewards could be sent to incorrect sponsor/winners
- Bypasses intended functionality.
Manual review, forge.
Add the following line inside distributeByOwner():
require(getProxyAddress(salt, implementation) == proxy);
Submitted by David77, 0x6980, oualidpro, 0x4ka5h, alymurtazamemon, 0xker2, castleChain, 0xRizwan, ZanyBonzy, Madalad, Tripathi, NoamYakov, 0x4non, 0xGovinda732, XVIronSec, sonny2k, lkjhgf, samshz, 0xWeb3boy, Bube, Silver Hawks, TheSchnilch, SAQ, golanger85, serialcoder. Selected submission by: serialcoder.
If the ProxyFactory::distributeByOwner() is executed before the Proxy contract has been deployed, the transaction will be executed successfully, but the stuck tokens will not be transferred to a rescue requestor.
The distributeByOwner() is used for recovering the rescue requestor's stuck tokens after a contest expires. The function will trigger the _distribute() to execute the contest's Proxy contract.
The transaction will not be reverted as expected if the Proxy has not been deployed before calling the distributeByOwner(). In other words, the transaction will be executed successfully, but the stuck tokens will not be transferred to the rescue requestor.
The root cause is that the _distribute() will make a low-level call to the Proxy without checking the existence of its contract.
function distributeByOwner(
address proxy,
address organizer,
bytes32 contestId,
address implementation,
bytes calldata data
) public onlyOwner {
if (proxy == address(0)) revert ProxyFactory__ProxyAddressCannotBeZero();
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
// distribute only when it exists and expired
if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
@> _distribute(proxy, data);
}
...
function _distribute(address proxy, bytes calldata data) internal {
@> (bool success,) = proxy.call(data);
if (!success) revert ProxyFactory__DelegateCallFailed();
@> emit Distributed(proxy, data);
}-
The distributeByOwner() triggers the _distribute(): https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L217 -
The _distribute() makes a low-level call without checking the existence of the Proxy's contract: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L250 -
The event Distributed will be emitted regardless of whether the Proxy has a contract: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L252
The rescue transaction will be executed successfully, but the stuck tokens will not be transferred to the rescue requestor. This incident can cause off-chain services to malfunction and confuse the owner (protocol admin) and the rescue requestor.
Manual Review
Verify that the target Proxy's address has a contract bytecode before executing the Proxy in the _distribute(), as below.
+ import {Address} from "openzeppelin/utils/Address.sol";
...
function _distribute(address proxy, bytes calldata data) internal {
+ if (!Address.isContract(proxy)) revert ProxyFactory__NoProxyContract();
(bool success,) = proxy.call(data);
if (!success) revert ProxyFactory__DelegateCallFailed();
emit Distributed(proxy, data);
}Submitted by Daniel526, tsvetanovv, Bughunter101, 0xbepresent, dacian, hunterw3b, VanGrim, 0xRizwan, 0xumarkhatab, RugpullDetector, 0xnevi, Cryptic Snake REACH, qpzm, Bauchibred, SanketKogekar. Selected submission by: dacian.
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L152-L160
The signature used in ProxyFactory::deployProxyAndDistributeBySignature() is missing a nonce & expiration deadline.
The signature used in ProxyFactory::deployProxyAndDistributeBySignature() is missing a nonce & expiration deadline.
This doesn't appear to currently be directly exploitable as ProxyFactory::_distribute() can't be called using the signature but without attempting to deploy the proxy. However the project team has stated they will be upgrading the contracts and that the current code is just an initial version, so best to point this out now as a low finding to prevent it from becoming a medium/high in a future version of the codebase.
Manual
Implement a nonce and an expiration deadline.
Submitted by PTolev, zach030, castleChain, JPCourses, 33BYTEZZZ, nadin, 0xDetermination, shikhar229169, 0xumarkhatab, IceBear, smbv1923, ubermensch, viking71, 0xlucky. Selected submission by: 33BYTEZZZ.
The identified vulnerability is associated with the _distribute function in
Distributor.sol. In scenarios where the total token amount is low or low values are used for percentages, the function may encounter a precision issue.
This arises due to the division of totalAmount by BASIS_POINTS to calculate the distribution amount for each winner. The precision error can lead to incorrect token distribution, affecting the fairness and accuracy of rewards to winners.
The vulnerability stems from the calculation of amount within the distribution loop. The formula amount = totalAmount * percentages[i] / BASIS_POINTS involves a division operation that could result in loss of precision(Rounding to Zero) when dealing with small totalAmount values or low percentages[i]. This imprecision can lead to token amounts being rounded down to zero, resulting in unfair or incomplete rewards for winners.
To simulate the vulnerability we need to make changes in the modifier
setUpContestForJasonAndSentJpycv2Token:
Code:
modifier setUpContestForJasonAndSentJpycv2Token(address _organizer) {
vm.startPrank(factoryAdmin);
bytes32 randomId = keccak256(abi.encode("Jason", "001"));
proxyFactory.setContest(_organizer, randomId, block.timestamp + 8 days, address(distributor));
vm.stopPrank();
bytes32 salt = keccak256(abi.encode(_organizer, randomId, address(distributor)));
address proxyAddress = proxyFactory.getProxyAddress(salt, address(distributor));
vm.startPrank(sponsor);
MockERC20(jpycv2Address).transfer(proxyAddress, 10);
vm.stopPrank();
// console.log(MockERC20(jpycv2Address).balanceOf(proxyAddress));
// assertEq(MockERC20(jpycv2Address).balanceOf(proxyAddress), 10000 ether);
_;
}
We change the value transferred to the proxyAddress to 10 tokens.
Note: We are not using 10 ether here as ether has 18 decimals which misguides the intended attack.
Now, we create a function called testPrecisionLoss() wherein we simulate end of a contest and call the deployProxyAndDistribute() function. This makes use of the modified createData() function to send in 95 winners, each being rewarded with percentage of 100 BASIS POINTS, which is equal to (10000 - COMMISSION_FEE) i.e. 9500 BASIS POINTS thus satisfying the conditions in the _distribute() function and allowing distribution of funds.
Code:
function testPrecisionLoss() public setUpContestForJasonAndSentJpycv2Token(organizer) {
bytes32 randomId_ = keccak256(abi.encode("Jason", "001"));
//create data from modified createData() function
bytes memory data = createData();
vm.startPrank(organizer);
console.log("User1 Start Balance -", MockERC20(jpycv2Address).balanceOf(user1));
console.log("Stadium Balance Before: ",MockERC20(jpycv2Address).balanceOf(stadiumAddress));
//warping to the time where contest ends and token distribution is allowed
vm.warp(30 days);
// distributing the rewards to all 95 winners
proxyFactory.deployProxyAndDistribute(randomId_, address(distributor), data);
console.log("Stadium End Balance: ",MockERC20(jpycv2Address).balanceOf(stadiumAddress));
console.log("User1 After Balance -", MockERC20(jpycv2Address).balanceOf(user1));
vm.stopPrank();
}
The logs prove the existence of precision loss:
Running 1 test for test/integration/ProxyFactoryTest.t.sol:ProxyFactoryTest
[PASS] testPrecisionLoss() (gas: 892788)
Logs:
0x000000000000000000000000000000000000000E
User1 Start Balance - 0
Stadium Balance Before: 0
Stadium End Balance: 10
User1 After Balance - 0
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.16ms
As we can see, all of the balance gets transferred to the stadiumAddress from the contract as the _commissionTransfer() function declares:
function _commissionTransfer(IERC20 token) internal {
token.safeTransfer(STADIUM_ADDRESS, token.balanceOf(address(this)));
}
Due to the precision loss(Rounding to Zero), none of the amount gets transferred to any winners and thus, all the "remaining tokens" get sent to the stadiumAddress, thus leaving our winners - in this case, user1 - rewarded with 0 balance.
The Rounding to Zero vulnerability has the potential to undermine the intended fairness and accuracy of the reward distribution process. In scenarios where the token balance is very small or percentages are low, the distribution algorithm could yield incorrect or negligible rewards to winners. This impacts the trust and credibility of the protocol, potentially leading to user dissatisfaction and decreased participation.
Manual Review Foundry VSCode
Consider instituting a predefined minimum threshold for the percentage amount used in the token distribution calculation. This approach ensures that the calculated distribution amount maintains a reasonable and equitable value, even when dealing with low percentages.
Additionally, an alternative strategy involves adopting a rounding up equation that consistently rounds the distribution amount upward to the nearest integer value.
By incorporating either of these methodologies, the precision vulnerability associated with small values and percentages can be effectively mitigated, resulting in more accurate and reliable token distribution outcomes.
Submitted by Kaveyjoe, ZedBlockchain, David77, Daniel526, Proxy, 0xyPhilic, nmirchev8, imkapadia, Lalanda, crippie, getitin, hunterw3b, thekmj, 33BYTEZZZ, SAAJ, 0xdraiakoo, xfu, Tripathi, 0xch, JohnnyTime, RugpullDetector, Cosine, sm4rty, leasowillow, honeymewn, radeveth, nervouspika, Maroutis, 0xVinylDavyl, Bauchibred, Gordoxyz, coditoorgeneral, 0xScourgedev, trauki. Selected submission by: Daniel526.
sparkn-contracts/src/Distributor.sol
Lines 144 to 152 in 9063a08
The _distribute function in the provided contract contains a loop that iterates through arrays of winners and percentages to distribute tokens. If these arrays are very large, this loop could lead to excessive gas consumption, potentially causing transactions to run out of gas and fail.
The _distribute function is responsible for distributing tokens to winners based on their percentages. This function iterates through arrays of winners and percentages, calculating the amount to transfer to each winner based on their percentage. While the function's purpose is to fairly distribute tokens, a potential vulnerability arises when dealing with a large number of winners and percentages.
function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
internal
{
// ...
uint256 winnersLength = winners.length;
for (uint256 i; i < winnersLength;) {
uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
erc20.safeTransfer(winners[i], amount);
unchecked {
++i;
}
}
// ...
}
The loop's gas cost increases linearly with the size of the winners and percentages arrays. If these arrays contain a significant number of elements, the gas consumption of the transaction could exceed the gas limit, causing the transaction to fail due to out-of-gas.
The impact of this issue is that transactions attempting to distribute tokens to a large number of winners in a single execution may fail due to running out of gas. Users may experience frustration and inconvenience if their intended distributions cannot be completed successfully.
Manual
Implement a batching mechanism that processes a limited number of winners and percentages in each iteration of the loop.
Submitted by 0xch13fd357r0y3r, 0xdeth, sv , InAllHonesty, GoSoul22, 0xnevi, savi0ur, Bughunter101, SBSecurity, alymurtazamemon, 0x3b, cats, nisedo, MrjoryStewartBaxter, KiteWeb3, Soliditors, 0xbepresent, DevABDee, Phantasmagoria, Breeje, FalconHoof, ABA, y4y, VanGrim, 0xanmol, Madalad, shikhar229169, TheSchnilch, 0xDetermination, Maroutis, Aamirusmani1552, zigtur, ke1caM, t0x1c, Stoicov, Chinmay, arnie, MehdiKarimi, 0xMosh, SAAJ, AkiraKodo, owade, 0xsandy, honeymewn, coolboymsk, trauki, 0xAxe, 0xScourgedev, 0x11singh99. Selected submission by: 0xAxe.
According to the provided link on "Centralization Risk for trusted owners", I believe that the Organizer also carries a centralization risk. As described in the documentation, "The sponsor Sponsor is the person providing financial support. Sponsors can be anyone, including the organizer Organizer. This implies that Organizer = Sponsor", which could potentially lead to unexpected situations.
- Anyone can become an
organizer, including thesponsor. This gives theorganizerexcessive power since one person can hold multiple roles, which could lead to malicious behavior, such as distributing rewards to acquaintances or oneself, prematurely ending the competition after obtaining a solution, or in the case ofsponsor = organizer, running away with the funds after obtaining a solution. - If
supportersdo not anonymize their submissions, it could result in covert operations. - Even though there is the possibility of off-chain identity verification for
organizers, I still see a significant level of susceptibility to manipulation within this protocol.
- Manual Review
- In my understanding, Sparkn is similar to the Immunefi auditing platform.
- My suggestion is to differentiate the roles of organizer and sponsor. Similar to the @codehawks platform, anonymize the solutions submitted by each supporter. The organizer can be any auditing platform (such as @code4rena, @sherlock, @codehawks), while the sponsor should only be the project itself, such as "sparkn" or "Beedle - Oracle free perpetual lending," and should not simultaneously hold the role of organizer.
Submitted by ZedBlockchain, carrotsmuggler, albertwhite, castleChain, SBSecurity, 0xHelium, Cosine, cats, MrjoryStewartBaxter, Stoicov, Batman, InAllHonesty, zach030, jonatascm, Chin, vjacs, Maroutis, Aamirusmani1552, B353N, MortezaXG38, Chandr, xAlismx, 419EF, owade, serverConnected, dipp, tsar, OxTenma, 0xAxe, 0xVinylDavyl. Selected submission by: tsar.
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol#L147
The distribute function does not check if the winner is address(0). For some tokens Like USDC and USDT it does check internally if the sender and receiver are not address(0) and revert it (so it's not necessary for the function to check it), but the DAI token does not check for that and will not revert and send tokens to the 0 address.
Since the dev described that the DAI token will be present in the contract the function _distribute should check if any of the winners are address(0).
Here is the DAI token code: https://etherscan.io/token/0x6b175474e89094c44da98b954eedeac495271d0f#code , the functions used to transfer the tokens internally does not check for the address(0) as seen here in the DAI contract:
function transfer(address dst, uint wad) external returns (bool) {
return transferFrom(msg.sender, dst, wad);
}
function transferFrom(address src, address dst, uint wad)
public returns (bool)
{
require(balanceOf[src] >= wad, "Dai/insufficient-balance");
if (src != msg.sender && allowance[src][msg.sender] != uint(-1)) {
require(allowance[src][msg.sender] >= wad, "Dai/insufficient-allowance");
allowance[src][msg.sender] = sub(allowance[src][msg.sender], wad);
}
balanceOf[src] = sub(balanceOf[src], wad);
balanceOf[dst] = add(balanceOf[dst], wad);
emit Transfer(src, dst, wad);
return true;
}DAI tokens could be permanently lost if sent to address(0) because lack of checking
Manual review, Etherscan
Add a built in check if any of the winners are 0 address because not all tokens do that check internally, specially DAI which the dev explicitly commented that it is going to be used.
Submitted by ZedBlockchain, SBSecurity, charlesCheerful, SAAJ, 97Sabit, Sabelo, mylifechangefast, sm4rty, contractsecure. Selected submission by: ZedBlockchain.
There are some critical functionalities that are missing events
-
Distributor.sol line 163 function _commissionTransfer(IERC20 token) should emit its own event, especially given that commissions transfers occur after distribution to winners. Its a critical function that can report to offchain tooling the commissions going to STADIUM at every moment
-
ProxyFactory.sol all deploy and distribute functions in the contract emit Distributed event. This is not truly reflective of happenings as they are different and events should reflect differences. The functions should emit specific events related to them e.g
- deployProxyAndDistributeByOwner //should differentiate that organizer not around so owner called this by having event with owner and organizer details emitted
- distributeByOwner // should differentiate that organizer called it , important information like above
- deployProxyAndDistribute are all functions with different intricacies and dynamics that can be captured by adding additional event or updating Distributed event to capture these differences
This shortchanges various offchain tooling, monitoring, reporting, frontend services that may rely on events to adequately capture real time activities of the contracts. It may even be critical for security monitoring so project can respond adequately if events sufficiently detailed and informative. Any emissions suspicious can allow protocol to react quickly
Manual Analysis
Recommended to add events for the cases detailed above e.g DistributeByOrganizer, DistributedSignature, DistributedOwner or keep Distributed and add other specific events in function e.g BySignatureEvent in function deployProxyAndDistributeBySignature() etc
Submitted by dontonka, oualidpro, Jarx, thekmj, castleChain, FalconHoof, codeslide, arnie, usmanfarooq90, WhiteRose, shirochan, leasowillow, SAQ, honeymewn, 0xVinylDavyl. Selected submission by: thekmj.
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol#L61
Using 10000 for BASIS_POINTS is not precise enough for the protocol use case.
The Distributor contract is intended to be used to distribute its own balance (i.e. prize pool) to recipients. The admin is expected to supply the list of winners, along with their percentage share of the prize pool. The contract will then distribute the prize to the recipients.
However, the percentage basis point is defined to be 10000, meaning the smallest possible prize pool denomination is 0.01%. We argue that this is not precise enough.
- Audit contests (e.g. in Code4rena, Sherlock, Codehawks) have prize pools to tens of thousands of USD worth. A standard contest is usually 50,000 USDC in prize. With 10000 as the basis points, winners can only be denominated to $5 of winnings only.
- SPARKN contest itself has a prize pool of $15,000. If any auditor's prize is not divisible by $1.5, then it is not possible to fairly distribute the prize for that auditor.
- It is common for a contest to have a finding with many duplicates, which payout is less than $1.
- It is also common for distributing events (e.g. airdrops, prize distribution) to have a percentage not divisible by 0.01%.
Therefore it will not be possible to distribute the prize with accuracy in such use-cases.
While it is technically possible to distribute the rewards using more than one sponsor transactions and equal number of distribution transactions, it will significantly complicate the reward calculation. In that case it will be better to just use standard ERC20 transferring to the winners, which defeats the purpose of the protocol to begin with. Therefore submitting as high.
It may not be possible to distribute rewards with high enough precision, blocking many realistic use cases.
Manual review
Use 10**18 for BASIS_POINTS instead of the current value, which should be precise enough.
Submitted by kaliberpoziomka, 0xkeesmark, shikhar229169, nmirchev8, JohnnyTime, JPCourses, 0xdraiakoo, Tripathi, niluke, ke1caM, 0xMosh, TheSchnilch, ryanjshaw, sonny2k, TorpedopistolIxc41, serialcoder, 0xScourgedev. Selected submission by: serialcoder.
The ProxyFactory::setContest() lacks validating the case of the implementation param pointing to a target address with no contract. As a result, all tokens sent to the Proxy address will be stuck forever.
The Proxy contract was designed as an escrow for distributing tokens to contest winners. By design, a contest organizer or sponsor must send tokens to an address of the contest's Proxy before the Proxy gets deployed. The tokens can be permanently stuck in the Proxy address if a mistake occurs.
Every contest will be initiated by an owner via the setContest(). The function will verify the implementation param against the address(0). However, the verification cannot guarantee that the Proxy will function properly.
Specifically, the setContest() lacks checking that the implementation param must point to an address with a contract bytecode. If an incorrect address is inputted by mistake, such as address(1), the Proxy will be bricked, locking away all tokens.
This mistake could not be undone since the inputted implementation param will be used to compute a salt for the Proxy. The implementation param will no longer be updated after executing the setContest().
function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
public
onlyOwner
{
@> if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();
if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {
revert ProxyFactory__CloseTimeNotInRange();
}
@> bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] != 0) revert ProxyFactory__ContestIsAlreadyRegistered();
saltToCloseTime[salt] = closeTime;
emit SetContest(organizer, contestId, closeTime, implementation);
}-
The implementation param is checked against the address(0) only: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L109 -
The implementation param is used to compute a salt for a contest's Proxy: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L113
To explain further the vulnerability.
The Proxy contract will be deployed and triggered by calling the following functions after sending prize tokens.
Below is the Proxy contract's snippet. The inputted implementation param will be assigned to the immutable variable _implementation in the constructor(). When the Proxy is invoked to distribute tokens, the fallback() will be triggered to execute the delegatecall() to the target implementation address.
If an incorrect address was inputted by mistake, the delegatecall() will execute nothing and return a success status. In other words, all tokens will be stuck forever in the Proxy contract.
...
@> address private immutable _implementation;
...
constructor(address implementation) {
@> _implementation = implementation;
}
...
fallback() external {
@> address implementation = _implementation;
assembly {
let ptr := mload(0x40)
calldatacopy(ptr, 0, calldatasize())
@> let result := delegatecall(gas(), implementation, ptr, calldatasize(), 0, 0)
let size := returndatasize()
returndatacopy(ptr, 0, size)
switch result
case 0 { revert(ptr, size) }
default { return(ptr, size) }
}
}-
The _implementation is an immutable variable initialized in the Proxy's constructor(): https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Proxy.sol#L40-L46 -
Proxy executes the delegatecall() to the target implementation address: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Proxy.sol#L52-L56
The setContest() lacks validating the case of the implementation param pointing to a target address with no contract. As a result, all tokens sent to the Proxy address will be stuck forever.
To clarify the vulnerability, although only an owner can execute the setContest() and the owner is trusted, the incident can occur by mistake (i.e., this vulnerability is not about any centralization or trust risks; it is about the risks of input mistakes only).
The likelihood is considered LOW (since the owner is expected to do due diligence). The impact is considered HIGH. Therefore, the severity is considered MEDIUM.
Manual Review
Further validating that the implementation param must point to an address with a contract bytecode, as follows.
+ import {Address} from "openzeppelin/utils/Address.sol";
...
function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
public
onlyOwner
{
if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();
+ if (!Address.isContract(implementation)) revert ProxyFactory__NoImplementationContract();
if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {
revert ProxyFactory__CloseTimeNotInRange();
}
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] != 0) revert ProxyFactory__ContestIsAlreadyRegistered();
saltToCloseTime[salt] = closeTime;
emit SetContest(organizer, contestId, closeTime, implementation);
}L-12. Organizers are not incentivized to deploy and distribute to winners causing that winners may not to be rewarded for a long time and force the protocol owner to manage the distribution
Submitted by 0xbepresent, Chinmay, 0xsandy, trauki. Selected submission by: 0xbepresent.
The organizer can deploy and distribute to winners at any time without restriction about the contest expiration time EXPIRATION_TIME causing that the winners to be unable to receive their rewards for a long time.
The organizer can execute the deployProxyAndDistribute() function to deploy the distribute contract and execute the distribution to winners. The only restriction is that the current time should be greater than contest close time (code line 134).
File: ProxyFactory.sol
127: function deployProxyAndDistribute(bytes32 contestId, address implementation, bytes calldata data)
128: public
129: returns (address)
130: {
131: bytes32 salt = _calculateSalt(msg.sender, contestId, implementation);
132: if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
133: // can set close time to current time and end it immediately if organizer wish
134: if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
...
...In the other hand, the owner can execute deployProxyAndDistributeByOwner() function after the contest expiration time (code line 187).
File: ProxyFactory.sol
179: function deployProxyAndDistributeByOwner(
180: address organizer,
181: bytes32 contestId,
182: address implementation,
183: bytes calldata data
184: ) public onlyOwner returns (address) {
185: bytes32 salt = _calculateSalt(organizer, contestId, implementation);
186: if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
187: if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
...
...The problem is that the organizer can execute the deployProxyAndDistribute() function after the contest close time without restriction of time. The organizer can wait indefinitely causing the winners not to be rewarded for a long time and force the owner to execute the distribution manually via deployProxyAndDistributeByOwner().
Additionally, the organizers are not incentivized to deploy and distribute to winners.
The malicious organizer can wait indefinitely until the owner calls deployProxyAndDistributeByOwner(). The bad/malicious behaivour of the organizer can cause the winners to be unable receive rewards for a long time AND force the owner to execute manually deployProxyAndDistributeByOwner(). That affects the protocol because rewards are not assigned in time AND the protocol owner needs to manage manually the deploy and distribution in order to not affect the protocol's reputation and winners.
Additionally the organizers are not incentivized to deploy and distribute to winners causing to the protocol owner to execute manually the deployProxyAndDistributeByOwner().
Manual review
Add a validation that the organizer distribution must be between the saltToCloseTime and the EXPIRATION_TIME. Same in deployProxyAndDistributeBySignature()
function deployProxyAndDistribute(bytes32 contestId, address implementation, bytes calldata data)
public
returns (address)
{
bytes32 salt = _calculateSalt(msg.sender, contestId, implementation);
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
// can set close time to current time and end it immediately if organizer wish
if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
++ if (saltToCloseTime[salt] + EXPIRATION_TIME < block.timestamp) revert();
address proxy = _deployProxy(msg.sender, contestId, implementation);
_distribute(proxy, data);
return proxy;
}Additionally, there should be a penalization to the organizer or an incentive to deploy and distribute in time to winners.