Skip to content

Commit

Permalink
update: fix some audit issues (#191)
Browse files Browse the repository at this point in the history 
* fix: add transferGas to updateParam method

* fix: change the storage slot of `transferGas` for compatibility
  • Loading branch information
@pythonberg1997
pythonberg1997 authored Sep 30, 2022
1 parent 9d45b31 commit a745ecc
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 20 deletions.
34 changes: 27 additions & 7 deletions contracts/Staking.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
uint256 public constant INIT_RELAYER_FEE = 16 * 1e15;
uint256 public constant INIT_BSC_RELAYER_FEE = 1 * 1e16;
uint256 public constant INIT_MIN_DELEGATION = 100 * 1e18;
uint256 public constant INIT_TRANSFER_GAS = 2300;

uint256 public relayerFee;
uint256 public bSCRelayerFee;
Expand All @@ -61,6 +62,8 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
uint256 internal rightIndex;
uint8 internal locked;

uint256 public transferGas; // this param is newly added after the hardfork on testnet. It need to be initialed by governed

modifier noReentrant() {
require(locked != 2, "No re-entrancy");
locked = 2;
Expand All @@ -78,6 +81,7 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
relayerFee = INIT_RELAYER_FEE;
bSCRelayerFee = INIT_BSC_RELAYER_FEE;
minDelegation = INIT_MIN_DELEGATION;
transferGas = INIT_TRANSFER_GAS;
alreadyInit = true;
}
_;
Expand Down Expand Up @@ -192,7 +196,8 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
function delegate(address validator, uint256 amount) override external payable noReentrant tenDecimalPrecision(amount) initParams {
require(amount >= minDelegation, "invalid delegate amount");
require(msg.value >= amount.add(relayerFee), "not enough msg value");
require(payable(msg.sender).send(0), "invalid delegator"); // the msg sender must be payable
(bool success,) = msg.sender.call{gas: transferGas}("");
require(success, "invalid delegator"); // the msg sender must be payable

uint256 convertedAmount = amount.div(TEN_DECIMALS); // native bnb decimals is 8 on BBC, while the native bnb decimals on BSC is 18
uint256 _relayerFee = (msg.value).sub(amount);
Expand All @@ -217,11 +222,14 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
function undelegate(address validator, uint256 amount) override external payable noReentrant tenDecimalPrecision(amount) initParams {
require(msg.value >= relayerFee, "not enough relay fee");
if (amount < minDelegation) {
require(amount > bSCRelayerFee, "not enough funds");
require(amount == delegatedOfValidator[msg.sender][validator], "invalid amount");
require(amount > bSCRelayerFee, "not enough funds");
}
require(delegatedOfValidator[msg.sender][validator] >= amount, "invalid amount");
require(block.timestamp >= pendingUndelegateTime[msg.sender][validator], "pending undelegation exist");
uint256 remainBalance = delegatedOfValidator[msg.sender][validator].sub(amount, "not enough funds");
if (remainBalance != 0) {
require(remainBalance > bSCRelayerFee, "insufficient balance after undelegate");
}

uint256 convertedAmount = amount.div(TEN_DECIMALS); // native bnb decimals is 8 on BBC, while the native bnb decimals on BSC is 18
uint256 _relayerFee = msg.value;
Expand All @@ -248,9 +256,13 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
function redelegate(address validatorSrc, address validatorDst, uint256 amount) override external noReentrant payable tenDecimalPrecision(amount) initParams {
require(validatorSrc != validatorDst, "invalid redelegation");
require(msg.value >= relayerFee, "not enough relay fee");
require(amount >= minDelegation && delegatedOfValidator[msg.sender][validatorSrc] >= amount, "invalid amount");
require(amount >= minDelegation, "invalid amount");
require(block.timestamp >= pendingRedelegateTime[msg.sender][validatorSrc][validatorDst] &&
block.timestamp >= pendingRedelegateTime[msg.sender][validatorDst][validatorSrc], "pending redelegation exist");
uint256 remainBalance = delegatedOfValidator[msg.sender][validatorSrc].sub(amount, "not enough funds");
if (remainBalance != 0) {
require(remainBalance > bSCRelayerFee, "insufficient balance after redelegate");
}

uint256 convertedAmount = amount.div(TEN_DECIMALS);// native bnb decimals is 8 on BBC, while the native bnb decimals on BSC is 18
uint256 _relayerFee = msg.value;
Expand All @@ -268,7 +280,7 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {

pendingRedelegateTime[msg.sender][validatorDst][validatorSrc] = block.timestamp.add(LOCK_TIME);
pendingRedelegateTime[msg.sender][validatorSrc][validatorDst] = block.timestamp.add(LOCK_TIME);

ICrossChain(CROSS_CHAIN_CONTRACT_ADDR).sendSynPackage(CROSS_STAKE_CHANNELID, msgBytes, oracleRelayerFee.div(TEN_DECIMALS));
payable(TOKEN_HUB_ADDR).transfer(oracleRelayerFee);
payable(SYSTEM_REWARD_ADDR).transfer(bSCRelayerFee);
Expand All @@ -281,7 +293,8 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
require(amount > 0, "no pending reward");

distributedReward[msg.sender] = 0;
payable(msg.sender).transfer(amount);
(bool success,) = msg.sender.call{gas: transferGas, value: amount}("");
require(success, "transfer failed");
emit rewardClaimed(msg.sender, amount);
}

Expand All @@ -290,7 +303,8 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
require(amount > 0, "no undelegated funds");

undelegated[msg.sender] = 0;
payable(msg.sender).transfer(amount);
(bool success,) = msg.sender.call{gas: transferGas, value: amount}("");
require(success, "transfer failed");
emit undelegatedClaimed(msg.sender, amount);
}

Expand Down Expand Up @@ -376,6 +390,7 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
} else if (Memory.compareStrings(key, "bSCRelayerFee")) {
require(value.length == 32, "length of bSCRelayerFee mismatch");
uint256 newBSCRelayerFee = BytesToTypes.bytesToUint256(32, value);
require(newBSCRelayerFee != 0, "the BSCRelayerFee must not be zero");
require(newBSCRelayerFee < relayerFee, "the BSCRelayerFee must be less than relayerFee");
require(newBSCRelayerFee%TEN_DECIMALS==0, "the BSCRelayerFee mod ten decimals must be zero");
bSCRelayerFee = newBSCRelayerFee;
Expand All @@ -384,6 +399,11 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
uint256 newMinDelegation = BytesToTypes.bytesToUint256(32, value);
require(newMinDelegation > relayerFee, "the minDelegation must be greater than relayerFee");
minDelegation = newMinDelegation;
} else if (Memory.compareStrings(key, "transferGas")) {
require(value.length == 32, "length of transferGas mismatch");
uint256 newTransferGas = BytesToTypes.bytesToUint256(32, value);
require(newTransferGas > 0, "the transferGas cannot be zero");
transferGas = newTransferGas;
} else {
revert("unknown param");
}
Expand Down
32 changes: 26 additions & 6 deletions contracts/Staking.template
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
uint256 public constant INIT_RELAYER_FEE = 16 * 1e15;
uint256 public constant INIT_BSC_RELAYER_FEE = 1 * 1e16;
uint256 public constant INIT_MIN_DELEGATION = 100 * 1e18;
uint256 public constant INIT_TRANSFER_GAS = 2300;

uint256 public relayerFee;
uint256 public bSCRelayerFee;
Expand All @@ -61,6 +62,8 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
uint256 internal rightIndex;
uint8 internal locked;

uint256 public transferGas; // this param is newly added after the hardfork on testnet. It need to be initialed by governed

modifier noReentrant() {
require(locked != 2, "No re-entrancy");
locked = 2;
Expand All @@ -78,6 +81,7 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
relayerFee = INIT_RELAYER_FEE;
bSCRelayerFee = INIT_BSC_RELAYER_FEE;
minDelegation = INIT_MIN_DELEGATION;
transferGas = INIT_TRANSFER_GAS;
alreadyInit = true;
}
_;
Expand Down Expand Up @@ -192,7 +196,8 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
function delegate(address validator, uint256 amount) override external payable noReentrant tenDecimalPrecision(amount) initParams {
require(amount >= minDelegation, "invalid delegate amount");
require(msg.value >= amount.add(relayerFee), "not enough msg value");
require(payable(msg.sender).send(0), "invalid delegator"); // the msg sender must be payable
(bool success,) = msg.sender.call{gas: transferGas}("");
require(success, "invalid delegator"); // the msg sender must be payable

uint256 convertedAmount = amount.div(TEN_DECIMALS); // native bnb decimals is 8 on BBC, while the native bnb decimals on BSC is 18
uint256 _relayerFee = (msg.value).sub(amount);
Expand All @@ -217,11 +222,14 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
function undelegate(address validator, uint256 amount) override external payable noReentrant tenDecimalPrecision(amount) initParams {
require(msg.value >= relayerFee, "not enough relay fee");
if (amount < minDelegation) {
require(amount > bSCRelayerFee, "not enough funds");
require(amount == delegatedOfValidator[msg.sender][validator], "invalid amount");
require(amount > bSCRelayerFee, "not enough funds");
}
require(delegatedOfValidator[msg.sender][validator] >= amount, "invalid amount");
require(block.timestamp >= pendingUndelegateTime[msg.sender][validator], "pending undelegation exist");
uint256 remainBalance = delegatedOfValidator[msg.sender][validator].sub(amount, "not enough funds");
if (remainBalance != 0) {
require(remainBalance > bSCRelayerFee, "insufficient balance after undelegate");
}

uint256 convertedAmount = amount.div(TEN_DECIMALS); // native bnb decimals is 8 on BBC, while the native bnb decimals on BSC is 18
uint256 _relayerFee = msg.value;
Expand All @@ -248,9 +256,13 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
function redelegate(address validatorSrc, address validatorDst, uint256 amount) override external noReentrant payable tenDecimalPrecision(amount) initParams {
require(validatorSrc != validatorDst, "invalid redelegation");
require(msg.value >= relayerFee, "not enough relay fee");
require(amount >= minDelegation && delegatedOfValidator[msg.sender][validatorSrc] >= amount, "invalid amount");
require(amount >= minDelegation, "invalid amount");
require(block.timestamp >= pendingRedelegateTime[msg.sender][validatorSrc][validatorDst] &&
block.timestamp >= pendingRedelegateTime[msg.sender][validatorDst][validatorSrc], "pending redelegation exist");
uint256 remainBalance = delegatedOfValidator[msg.sender][validatorSrc].sub(amount, "not enough funds");
if (remainBalance != 0) {
require(remainBalance > bSCRelayerFee, "insufficient balance after redelegate");
}

uint256 convertedAmount = amount.div(TEN_DECIMALS);// native bnb decimals is 8 on BBC, while the native bnb decimals on BSC is 18
uint256 _relayerFee = msg.value;
Expand Down Expand Up @@ -281,7 +293,8 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
require(amount > 0, "no pending reward");

distributedReward[msg.sender] = 0;
payable(msg.sender).transfer(amount);
(bool success,) = msg.sender.call{gas: transferGas, value: amount}("");
require(success, "transfer failed");
emit rewardClaimed(msg.sender, amount);
}

Expand All @@ -290,7 +303,8 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
require(amount > 0, "no undelegated funds");

undelegated[msg.sender] = 0;
payable(msg.sender).transfer(amount);
(bool success,) = msg.sender.call{gas: transferGas, value: amount}("");
require(success, "transfer failed");
emit undelegatedClaimed(msg.sender, amount);
}

Expand Down Expand Up @@ -376,6 +390,7 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
} else if (Memory.compareStrings(key, "bSCRelayerFee")) {
require(value.length == 32, "length of bSCRelayerFee mismatch");
uint256 newBSCRelayerFee = BytesToTypes.bytesToUint256(32, value);
require(newBSCRelayerFee != 0, "the BSCRelayerFee must not be zero");
require(newBSCRelayerFee < relayerFee, "the BSCRelayerFee must be less than relayerFee");
require(newBSCRelayerFee%TEN_DECIMALS==0, "the BSCRelayerFee mod ten decimals must be zero");
bSCRelayerFee = newBSCRelayerFee;
Expand All @@ -384,6 +399,11 @@ contract Staking is IStaking, System, IParamSubscriber, IApplication {
uint256 newMinDelegation = BytesToTypes.bytesToUint256(32, value);
require(newMinDelegation > relayerFee, "the minDelegation must be greater than relayerFee");
minDelegation = newMinDelegation;
} else if (Memory.compareStrings(key, "transferGas")) {
require(value.length == 32, "length of transferGas mismatch");
uint256 newTransferGas = BytesToTypes.bytesToUint256(32, value);
require(newTransferGas > 0, "the transferGas cannot be zero");
transferGas = newTransferGas;
} else {
revert("unknown param");
}
Expand Down
4 changes: 3 additions & 1 deletion contracts/TokenHub.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,9 @@ contract TokenHub is ITokenHub, System, IParamSubscriber, IApplication, ISystemR

function withdrawStakingBNB(uint256 amount) external override returns(bool) {
require(msg.sender == STAKING_CONTRACT_ADDR, "only staking system contract can call this function");
payable(STAKING_CONTRACT_ADDR).transfer(amount);
if (amount != 0) {
payable(STAKING_CONTRACT_ADDR).transfer(amount);
}
return true;
}
}
4 changes: 3 additions & 1 deletion contracts/TokenHub.template
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,9 @@ contract TokenHub is ITokenHub, System, IParamSubscriber, IApplication, ISystemR

function withdrawStakingBNB(uint256 amount) external override returns(bool) {
require(msg.sender == STAKING_CONTRACT_ADDR, "only staking system contract can call this function");
payable(STAKING_CONTRACT_ADDR).transfer(amount);
if (amount != 0) {
payable(STAKING_CONTRACT_ADDR).transfer(amount);
}
return true;
}
}
9 changes: 4 additions & 5 deletions contracts/mock/MockTokenHub.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,10 @@ contract MockTokenHub is ITokenHub {

function withdrawStakingBNB(uint256 amount) external override returns(bool) {
address STAKING_CONTRACT_ADDR = address(0x0000000000000000000000000000000000002001);
require(msg.sender == STAKING_CONTRACT_ADDR,
"only staking system contract can call this function");
payable(STAKING_CONTRACT_ADDR).transfer(amount);
require(msg.sender == STAKING_CONTRACT_ADDR, "only staking system contract can call this function");
if (amount != 0) {
payable(STAKING_CONTRACT_ADDR).transfer(amount);
}
return true;
}
}


0 comments on commit a745ecc

Please sign in to comment.