cyfrin / 2023-09-sparkn-mitigation2 Goto Github PK
View Code? Open in Web Editor NEWLicense: Other
License: Other
Link to Issue: Lack of checking the existence of the Proxy contract
The pull request fixes the issue reported, correctly verifying that the destination address code length is different than 0, using the recommendation of Issue 787 from the contest.
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
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#H-01
Fix PR : codefox-inc/sparkn-contracts#30
Fixed.
The fix adds implementaion
in the signature preventing the usage of a signature on multiple implementaions.
The pull request fixes the issue reported, adding the implementation address to the signature hash, using the recommendation given in the selected issue.
However, the line added contains an error explained in #1
L-11 : Insufficient validation leads to locking up prize tokens forever
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-11
Fix PR: codefox-inc/sparkn-contracts#34
Not completely fixed.
This PR is supposed to fix 2 issues in the code:
implementation
parameter in the setContest
function.The fundamental issue here is the creation of a proxy contract from which tokens cannot be moved by the protcol due to incorrect implementation
being passed at the time of contest creation.
The code added to resolve this issue is:
@@ -107,6 +115,7 @@ contract ProxyFactory is Ownable, EIP712 {
onlyOwner
{
if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();
+ if (implementation.code.length == 0) revert ProxyFactory__ImplementationNotDeployed();
if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {
revert ProxyFactory__CloseTimeNotInRange();
}
This check still allows all contract addresses that is not an implementation
of SPARKN to be set for the contest which will result in the same issue.
A much safer validation method would be to store the valid implementations in a mapping and then compare against it. This also saves gas.
diff --git a/src/ProxyFactory.sol b/src/ProxyFactory.sol
index d7a1960..0bd8575 100644
--- a/src/ProxyFactory.sol
+++ b/src/ProxyFactory.sol
@@ -77,6 +77,7 @@ contract ProxyFactory is Ownable, EIP712 {
mapping(bytes32 => uint256) public saltToCloseTime;
/// @dev record whitelisted tokens
mapping(address => bool) public whitelistedTokens;
+ mapping(address => uint256) public implementations;
////////////////////////////
/////// Constructor ////////
@@ -100,6 +101,9 @@ contract ProxyFactory is Ownable, EIP712 {
////////////////////////////////////////////
/////// External & Public functions ////////
////////////////////////////////////////////
+ function addImplementation(address implementation) external onlyOwner {
+ implementations[implementation] = 1;
+ }
/**
* @notice Only owner can set contest's properties
* @notice close time must be less than 28 days from now
@@ -110,12 +114,13 @@ contract ProxyFactory is Ownable, EIP712 {
* @param closeTime The contest close time
* @param implementation The implementation address
*/
+
function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
public
onlyOwner
{
- if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();
- if (implementation.code.length == 0) revert ProxyFactory__ImplementationNotDeployed();
+ if (organizer == address(0)) revert ProxyFactory__NoZeroAddress();
+ if (implementations[implementation] == 0) revert ProxyFactory__ImplementationNotDeployed();
if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {
revert ProxyFactory__CloseTimeNotInRange();
}
getProxyAddress
functionThe issue here is that a sponsor/front-end mistakenly passing incorrect arguments to the getProxyAddress
could result in the user sending funds to a wrong address which might not be recoverable.
The code added to resolve this issue is:
@@ -223,6 +233,7 @@ contract ProxyFactory is Ownable, EIP712 {
/// @param implementation The implementation address
/// @return proxy The calculated proxy address
function getProxyAddress(bytes32 salt, address implementation) public view returns (address proxy) {
+ if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
bytes memory code = abi.encodePacked(type(Proxy).creationCode, uint256(uint160(implementation)));
bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, keccak256(code)));
proxy = address(uint160(uint256(hash)));
This guards against non-existing salts but still allows all other invalid combinations of salt
and implementation
.
This issue cannot be fully prevented as a valid proxy address could still be an incorrect address for the user. Trying to improvise the getProxyAddress()
function in the context of user's safety will require incurring unwanted gas costs in entirety. Even the current additional check added in the getProxyAddress()
has devoid caching the variable in distributeByOwner()
function.
A seperate function that takes in organizer
, contestId
and implementation
as the parameters and computes the proxy
address will offer the maximum protection possible against such issues without adding extra gas cost to the other functionalities. Also with the current implementation of getProxyAddress()
, the user has to pass in the salt
which is not emitted via an event or calculatable on-chain.
@@ -239,6 +244,15 @@ contract ProxyFactory is Ownable, EIP712 {
proxy = address(uint160(uint256(hash)));
}
+ function getProxyAddressSafely(address organizer, bytes32 contestId, address implementation)
+ external
+ view
+ returns (address proxy)
+ {
+ bytes32 salt = _calculateSalt(organizer, contestId, implementation);
+ proxy = getProxyAddress(salt, implementation);
+ }
+
L-09 : Missing Events
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-09
Acknowledged not fixed.
The rationale for not choosing to fix provided is: (codefox-inc/sparkn-contracts#21)
1. Not necessary for now
L-06. Potential DOS due to Gas Exhaustion Due to Large Array Iteration in _distribute Function
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-06
Acknowledged not fixed.
The rationale for not choosing to fix provided is: (codefox-inc/sparkn-contracts#27)
1. The loop is supposed not to be very long.
- Usually its length is 5.
The team plans to not have a large set of winners in which case there would be no issues.
M-01 : The digest calculation in deployProxyAndDistributeBySignature does not follow EIP-712 specification
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#M-01
Fix PR : codefox-inc/sparkn-contracts#29
Not Fixed.
EIP-712 requires dynamic types to be encoded as the hash of its content when calculating the digest. The updated code still encodes bytes data
as bytes
.
Update the code in the deployProxyAndDistributeBySignature()
function
https://github.com/codefox-inc/sparkn-contracts/blob/ec4d336d0df7c927e4769860717716a9f3b4199f/src/ProxyFactory.sol#L168C1-L170C104
diff --git a/src/ProxyFactory.sol b/src/ProxyFactory.sol
index d7a1960..c77559a 100644
--- a/src/ProxyFactory.sol
+++ b/src/ProxyFactory.sol
@@ -165,8 +165,9 @@ contract ProxyFactory is Ownable, EIP712 {
bytes calldata signature,
bytes calldata data
) public returns (address) {
- bytes32 digest =
- _hashTypedDataV4(keccak256(abi.encode(_DEPLOY_AND_DISTRIBUTE_TYPEHASH, contestId, implementation, data)));
+ bytes32 digest = _hashTypedDataV4(
+ keccak256(abi.encode(_DEPLOY_AND_DISTRIBUTE_TYPEHASH, contestId, implementation, keccak256(data)))
+ );
if (!organizer.isValidSignatureNow(digest, signature)) revert ProxyFactory__InvalidSignature();
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
Issue is correctly mitigated as per the recommendations.
I find it unlikely that
winners[i]
can be 0 given that this is a value that comes through either the Owner or an Organizer, but the check does not hurt.
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
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-12
Acknowledged not fixed.
The rationale for not fixing provided is: (codefox-inc/sparkn-contracts#24)
1. This can be a problem in the long turn, which can pose a threat on the health of the protocol. But it is not an issue for now.
This issue deals with a malicious organizer in which case even severe expolits like M-03 are possible. As of the current design the organizer is blindly trusted with the distribution of the prizes.
L-07 : Centralization Risk for trusted organizers
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-07
Acknowledged not fixed.
The rationale for not choosing to fix provided is: (codefox-inc/sparkn-contracts#28)
1. you cannot stop someone from sending ERC20 tokens to proxy address.
2. sponsor can be anyone is by design
The vulnerability intended in the report seems to be not about the organizer's ability to fund a contest but the ability of a sponsor ( here the term as used by the reportee seems to mean the main entity behind the project ) to be the organizer and act maliciously. This would be then similiar to M-03 which was decided by the team to not be considered now.
M-03 : Malicious/Compromised organiser can reclaw all funds, stealing work from supporters
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#M-03
Acknowledged decided not to fix.
The rationale for not fixing provided is: (codefox-inc/sparkn-contracts#18)
1. There is no way to stop this from occurring at the moment only from the contract level.
2. We will handle this in the future though.
Here the main object of trust is the organizer which can be anybody. To better this would require some high level decisions (eg: instead shifting the trust to owner in distribution of prizes etc.) to be made in choosing the winner's of a contest which is up to the protocol team.
As of the current design, the organizer is blindly trusted in distributing the prizes fairly.
Link to Issue: DAI Tokens at Risk Due to Lack of address(0) Check in distribute
The pull request fixes the issue reported, correctly verifying if the destination address of a ERC20 is not the address 0 and preventing the loss of funds.
As a new feature ERC1271 signature validation is added
Feature PR : codefox-inc/sparkn-contracts#16
No issues
Link to Issue: Blacklisted STADIUM_ADDRESS address cause fund stuck in the contract forever
The issue was marked as acknowledged and decided not to fix it. With the following reasons. However, while we agree that the blacklist probability is low, if it happens then redeploying the contracts is not enough, since implementations for already registered contests have this address fixed.
Implement a getter method for STADIUM_ADDRESS
in ProxyFactory.sol
making it variable but keeping the immutability of Distributor.sol
.
...
event SetContest(
address indexed organizer, bytes32 indexed contestId, uint256 closeTime, address indexed implementation
);
+ event SetStadiumAddress(address indexed stadiumAddress);
event Distributed(address indexed proxy, bytes data);
...
// contest distribution expiration
bytes32 internal constant _DEPLOY_AND_DISTRIBUTE_TYPEHASH =
keccak256("DeployAndDistribute(bytes32 contestId,address implementation,bytes data)");
uint256 public constant EXPIRATION_TIME = 7 days;
uint256 public constant MAX_CONTEST_PERIOD = 28 days;
+ address public _stadiumAddress;
...
+ /**
+ * @notice Only owner can set stadium address
+ * @dev Set stadium address
+ * @param _newStadiumAddress The new stadium address
+ */
+ function setStadiumAddress(address _newStadiumAddress) public onlyOwner {
+ _stadiumAddress = _newStadiumAddress;
+ emit SetStadiumAddress(_newStadiumAddress);
+ }
...
...
uint8 private constant VERSION = 1; // version is 1 for now
address private immutable FACTORY_ADDRESS;
- address private immutable STADIUM_ADDRESS;
uint256 private constant COMMISSION_FEE = 500; // this can be changed in the future
// a constant value of 10,000 (basis points) = 100%
uint256 private constant BASIS_POINTS = 10000;
...
/// @dev initiate the contract with factory address and other key addresses, fee rate
constructor(
// uint256 version, // for future use
- address factoryAddress,
- address stadiumAddress
+ address factoryAddress
)
/* solhint-enable */
{
- if (factoryAddress == address(0) || stadiumAddress == address(0)) revert Distributor__NoZeroAddress();
+ if (factoryAddress == address(0)) revert Distributor__NoZeroAddress();
FACTORY_ADDRESS = factoryAddress; // initialize with deployed factory address beforehand
- STADIUM_ADDRESS = stadiumAddress; // official address to receive commission fee
}
...
function _commissionTransfer(IERC20 token) internal {
- token.safeTransfer(STADIUM_ADDRESS, token.balanceOf(address(this)));
+ token.safeTransfer(getStadiumAddress(), token.balanceOf(address(this)));
}
...
/**
* @notice returns all the immutable and constant addresses and values
* @dev This function is for convenience to check the addresses and values
*/
function getConstants()
external
view
returns (address _FACTORY_ADDRESS, address _STADIUM_ADDRESS, uint256 _COMMISSION_FEE, uint8 _VERSION)
{
/* solhint-disable */
_FACTORY_ADDRESS = FACTORY_ADDRESS;
- _STADIUM_ADDRESS = STADIUM_ADDRESS;
+ _STADIUM_ADDRESS = getStadiumAddress();
_COMMISSION_FEE = COMMISSION_FEE;
_VERSION = VERSION;
/* solhint-enable */
}
+
+ /**
+ * @notice returns stadium address from proxy factory
+ * @dev This function is for convenience to get the stadium address
+ */
+ function getStadiumAddress() internal view returns (address) {
+ return ProxyFactory(FACTORY_ADDRESS)._stadiumAddress();
+ }
L-04 : Signature missing nonce & expiration deadline
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-04
Acknowledged not fixed.
The rationale for not choosing to fix provided is: (codefox-inc/sparkn-contracts#25)
Although there is no expiration and nonce, "contestId being unique and proxy deployment can be done once" can be a substitute for them.
As of the current implementation it is not an issue.
L-08 : DAI Tokens at Risk Due to Lack of address(0) Check in distribute
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-08
Fix PR : codefox-inc/sparkn-contracts#32
Fixed.
In the fix, the winner address is being validated for 0 which resolves this issue.
Link to Issue: Owner can incorrectly pull funds from contests not yet expired
The pull request fixes the issue, but there is some room for a gas optimization. Instead of comparing the proxy address sent against the one calculated with the salt and implementation, just use the one computed by getProxyAddress
and remove two checks and one parameter.
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 (proxy != getProxyAddress(salt, implementation)) {
- revert ProxyFactory__ProxyAddressMismatch();
- }
// distribute only when it exists and expired
if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
+ address proxy = getProxyAddress(salt, implementation);
_distribute(proxy, data);
}
L-05 : Precision loss/Rounding to Zero in _distribute()
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-05
Acknowledged not fixed.
The rationale for not choosing to fix provided is: (codefox-inc/sparkn-contracts#26)
1. there will only be several winners with not too small percentages.
2. And if the prize is like 0.01 JPYC, the precision loss is negligible.
If the combination of prize token amount and percentage is taken care of as mentioned, it wouldn't cause an issue.
L-01 : If a winner is blacklisted on any of the tokens they can't receive their funds
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-01
Acknowledged decided not to fix.
The rationale for not choosing to fix provided is: (codefox-inc/sparkn-contracts#19)
1. somehow we can handle this off-chain if it happens
2. this is really a rare case to take into account
The organizer chooses the winners off-chain at which moment the validation for the blacklisted user's can occur following which a decision inline with the organizer/protocol ( eg: the blacklisted user's will not receive any funds or the option for the blacklisted user's to update their address within x time etc.) can be made offline. Hence as stated by the team, the issue can be handled off-chain.
L-10 : Using basis points for percentage is not precise enough for realistic use-cases
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-10
Acknowledged not fixed.
In case the team doesn't plan to support distributing prizes at less than 0.01%, it should not cause any issue.
The mitigation adds a new line in which the implementation
parameter is checked to have some code deployed in it. This LGTM, but I have a potential suggestion to make this even safer.
function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
public
onlyOwner
{
...
if (implementation.code.length == 0) revert ProxyFactory__ImplementationNotDeployed();
...
emit SetContest(organizer, contestId, closeTime, implementation);
}
Instead of checking the code length, you could instead create a specific interface that all Distributors (current and future ones) must implement to register themselves as valid potential distributors and make sure there is not just any code but rather code capable of dealing with the tokens it will hold.
You could just implement your own interface manually.
Or adhere to the EIP-165
standard and use the supportsInterface()
function (Use the OZUpgradeable version for the proxies):
// @ DistributorInterface
// Create an interface for current and future distributor implementations
// For this example, a simple view function could be enough
interface DistributorInterface {
function isDistributor() public view returns (bool);
}
// @ Distributor.sol
// Implement OZ's ERC165
contract Distributor is ERC165 {
function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
return interfaceId == type(DistributorInterface).interfaceId || super.supportsInterface(interfaceId);
}
}
// @ ProxyFactory.sol
// Check that the implementation supports the expected interface. In this case, validating that code exists and that
// the expected functions to handle the reward distribution are present.
function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
public
onlyOwner
{
if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();
- if (implementation.code.length == 0) revert ProxyFactory__ImplementationNotDeployed();
+ if (ERC165Checker.supportsInterface(implementation, type(DistributorInterface).interfaceId)) revert ProxyFactory__ImplementationInvalid();
...
emit SetContest(organizer, contestId, closeTime, implementation);
}
This example defines a very generic interface which, for the purposes of the check in setContest
, may be enough. But you could definitely make it more dynamic and not hard-code the expected interface (and thus use fully-fledged interfaces when implementing new distributors) and not have to re-deploy ProxyFactory
.
There is a case to be made about this suggestion adding “unnecessary” additional logic / complexity to the codebase, in which case then the original mitigation is probably the most straight-forward option. Nevertheless, I figured I’d still share my two cents.
This issue is outside of contest report
Simplify the loop : codefox-inc/sparkn-contracts#8
Fixed.
The fix combines iteration of two arrays into a single loop hence saving gas cost.
More gas can be saved within the function without major changes:
winners
length earlierwinners[i]
and percentages[i]
in the loopdiff --git a/src/Distributor.sol b/src/Distributor.sol
index 876af3b..a0f2742 100644
--- a/src/Distributor.sol
+++ b/src/Distributor.sol
@@ -88,7 +88,7 @@ contract Distributor {
* @param winners The addresses array of winners
* @param percentages The percentages array of winners
*/
- function distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
+ function distribute(address token, address[] calldata winners, uint256[] calldata percentages, bytes calldata data)
external
{
if (msg.sender != FACTORY_ADDRESS) {
@@ -112,7 +112,7 @@ contract Distributor {
* @param percentages The percentages of winners
* @param data The data to be logged. It is supposed to be used for showing the realation bbetween winners and proposals.
*/
- function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
+ function _distribute(address token, address[] calldata winners, uint256[] calldata percentages, bytes calldata data)
internal
{
// token address input check
@@ -121,7 +121,8 @@ contract Distributor {
revert Distributor__InvalidTokenAddress();
}
// winners and percentages input check
- if (winners.length == 0 || winners.length != percentages.length) revert Distributor__MismatchedArrays();
+ uint256 winnersLength = winners.length; // cache length
+ if (winnersLength == 0 || winnersLength != percentages.length) revert Distributor__MismatchedArrays();
// prepare for the loop
IERC20 erc20 = IERC20(token);
@@ -129,15 +130,19 @@ contract Distributor {
uint256 totalAmount = erc20.balanceOf(address(this));
// if there is no token to distribute, then revert
if (totalAmount == 0) revert Distributor__NoTokenToDistribute();
-
+
// percentages.length = winners length
- uint256 winnersLength = winners.length; // cache length
+
uint256 totalPercentage;
for (uint256 i; i < winnersLength;) {
- totalPercentage += percentages[i];
- uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
- if (winners[i] == address(0)) revert Distributor__NoZeroAddress();
- erc20.safeTransfer(winners[i], amount);
+ uint256 percentage = percentages[i];
+
+ totalPercentage += percentage;
+ uint256 amount = totalAmount * percentage / BASIS_POINTS;
+
+ address winner = winners[i];
+ if (winner == address(0)) revert Distributor__NoZeroAddress();
+ erc20.safeTransfer(winner, amount);
unchecked {
++i;
}
The current mitigation still does not properly implement the EIP-712 standard.
The function typehash
has been added to the encoding of the digest
calculation, which was indeed missing from the standard.
However, the digest
is still being calculated erroneously, as the dynamic parameter data
is still not being hashed, which the specification establishes.
function deployProxyAndDistributeBySignature(...) public returns (address) {
bytes32 digest =
_hashTypedDataV4(
keccak256(
abi.encode(
_DEPLOY_AND_DISTRIBUTE_TYPEHASH,
contestId,
implementation,
keccak256(data)
)
)
);
...
}
I suggest revisiting the related issue for the corresponding links to the EIP that explain the specification for dynamic values such as bytes
M-02 : Blacklisted STADIUM_ADDRESS address cause fund stuck in the contract forever
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#M-02
Acknowledged decided not to fix.
The rationale for not fixing provided is: (codefox-inc/sparkn-contracts#17)
1. possibility of getting blacklisted is very low.
2. complexity increases.
3. can get upgraded by redeploy the factory and distributor contract.
It is very valid that under the condition of a trusted STADIUM_ADDRESS the probability of getting blacklisted is extremely low. But the proposed move of upgrading by redeployment of the factory and distributor contract
will lead to loss of funds for all the funded contests whose prizes have not yet been distributed.
L-02 : Owner can incorrectly pull funds from contests not yet expired
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-02
Fix PR : codefox-inc/sparkn-contracts#31
Fixed.
The issue arose due to the proxy
parameter not being linked with the other params in the distributeByOwner
function allowing the owner to pass in any arbitrary proxy address. In the fix, the proxy
parameter is validated by retrieving the proxy address associated with the other params.
Gas can be saved by using the proxy address obtained from getProxyAddress()
straightaway instead of inputting from the owner and performing validation.
diff --git a/src/ProxyFactory.sol b/src/ProxyFactory.sol
index d7a1960..8156679 100644
--- a/src/ProxyFactory.sol
+++ b/src/ProxyFactory.sol
@@ -204,26 +204,22 @@ contract ProxyFactory is Ownable, EIP712 {
* @notice Owner can rescue funds if token is stuck after the deployment and contest is over for a while
* @dev only owner can call this function and it is supposed not to be called often
* @dev fee sent to stadium address is included in the logic contract
- * @param proxy The proxy address
* @param organizer The contest organizer
* @param contestId The contest id
* @param implementation The implementation address
* @param data The prize distribution calling data
*/
function distributeByOwner(
- address proxy,
+ // 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 (proxy != getProxyAddress(salt, implementation)) {
- revert ProxyFactory__ProxyAddressMismatch();
- }
// distribute only when it exists and expired
if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
+ address proxy = getProxyAddress(salt, implementation);
_distribute(proxy, data);
}
L-03 : Lack of checking the existence of the Proxy contract
Issue Link : https://www.codehawks.com/report/cllcnja1h0001lc08z7w0orxx#L-03
Fix PR : codefox-inc/sparkn-contracts#33
Fixed.
Fixed by checking if the code size is 0 before making the call in the _distribute() function.
Gas can be saved by placing this check in the distributeByOwner() instead. All other functions which call _distribute() themselves deploys the proxy, making this an unwanted check.
diff --git a/src/ProxyFactory.sol b/src/ProxyFactory.sol
index d7a1960..7662cd8 100644
--- a/src/ProxyFactory.sol
+++ b/src/ProxyFactory.sol
@@ -224,6 +224,7 @@ contract ProxyFactory is Ownable, EIP712 {
}
// distribute only when it exists and expired
if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
+ if (proxy.code.length == 0) revert ProxyFactory__ProxyIsNotAContract();
_distribute(proxy, data);
}
@@ -259,7 +260,6 @@ contract ProxyFactory is Ownable, EIP712 {
/// @param proxy The proxy address
/// @param data The prize distribution data
function _distribute(address proxy, bytes calldata data) internal {
- if (proxy.code.length == 0) revert ProxyFactory__ProxyIsNotAContract();
(bool success,) = proxy.call(data);
if (!success) revert ProxyFactory__DelegateCallFailed();
emit Distributed(proxy, data);
Link to Issue: The digest
calculation in deployProxyAndDistributeBySignature does not follow EIP-712 specification
The issue is partially fixed, hashing dynamic value of data
is missing. As EPI712 mentions: "...dynamic values bytes and string are encoded as a keccak256 hash of their contents."
The proposed pull request modifies the code to follow hashStruct(message)
specification. However, the hashing of data
, as mentioned before, is missing.
_hashTypedDataV4(keccak256(abi.encode(_DEPLOY_AND_DISTRIBUTE_TYPEHASH, contestId, implementation, data)));
EIP-712 structured data hashing standard is not properly implemented.
function deployProxyAndDistributeBySignature(
address organizer,
bytes32 contestId,
address implementation,
bytes calldata signature,
bytes calldata data
) public returns (address) {
bytes32 digest =
- _hashTypedDataV4(keccak256(abi.encode(_DEPLOY_AND_DISTRIBUTE_TYPEHASH, contestId, implementation, data)));
+ _hashTypedDataV4(
+ keccak256(
+ abi.encode(
+ _DEPLOY_AND_DISTRIBUTE_TYPEHASH,
+ contestId,
+ implementation,
+ keccak256(data)
+ )
+ )
+ );
if (!organizer.isValidSignatureNow(digest, signature)) revert ProxyFactory__InvalidSignature();
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
address proxy = _deployProxy(organizer, contestId, implementation);
_distribute(proxy, data);
return proxy;
}
Issue is correctly mitigated as per the recommendations.
May be worth considering #6 for this too.
Issue is correctly mitigated as per the recommendations, reducing the replayability of the signature.
Make sure this is still kept after fixing #5 , as both issues involve the same line of code.
Issue is correctly mitigated as per the recommendations.
In Distributor.sol
line #119 that contains an check for address zero, can be removed safely since after it there is whitelisted tokens control that will halt the execution unless address zero is whitelisted.
function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
internal
{
// token address input check
if (token == address(0)) revert Distributor__NoZeroAddress();
if (!_isWhiteListed(token)) {
revert Distributor__InvalidTokenAddress();
}
// winners and percentages input check
if (winners.length == 0 || winners.length != percentages.length) revert Distributor__MismatchedArrays();
...
}
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.