Skip to content

Commit

Permalink
Fix addition gas in external calls (#195)
Browse files Browse the repository at this point in the history
* fix contracts

* add fallback test

* Fix gas addition gas to be 1200 (3500 in total)

* Remove gas striction between cross-contract call

* remove console

* revert garbage
  • Loading branch information
nxqbao authored Mar 10, 2023
1 parent fe27e4f commit 0dd2ec4
Show file tree
Hide file tree
Showing 10 changed files with 178 additions and 19 deletions.
10 changes: 10 additions & 0 deletions contracts/extensions/consumers/GlobalConfigConsumer.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.9;

abstract contract GlobalConfigConsumer {
/// @dev The addition amount of gas sending along in external calls. Total gas stipend is added with default 2300 gas.
uint256 public constant DEFAULT_ADDITION_GAS = 1200;
/// @dev The length of a period in second.
uint256 public constant PERIOD_DURATION = 1 days;
}
5 changes: 3 additions & 2 deletions contracts/mocks/MockStaking.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

pragma solidity ^0.8.9;

import "../extensions/consumers/GlobalConfigConsumer.sol";
import "../ronin/staking/RewardCalculation.sol";

contract MockStaking is RewardCalculation {
contract MockStaking is RewardCalculation, GlobalConfigConsumer {
/// @dev Mapping from user => staking balance
mapping(address => uint256) internal _stakingAmount;
/// @dev Mapping from period number => slashed
Expand All @@ -22,7 +23,7 @@ contract MockStaking is RewardCalculation {

function firstEverWrapup() external {
delete pendingReward;
lastUpdatedPeriod = block.timestamp / 1 days + 1;
lastUpdatedPeriod = block.timestamp / PERIOD_DURATION + 1;
}

function endPeriod() external {
Expand Down
44 changes: 44 additions & 0 deletions contracts/mocks/MockTransferFallback.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.9;

import "../extensions/RONTransferHelper.sol";

contract MockPaymentFallback {
event SafeReceived(address indexed sender, uint256 value);

/// @dev Fallback function accepts ether transactions.
receive() external payable {
emit SafeReceived(msg.sender, msg.value);
}
}

contract MockPaymentFallbackExpensive {
uint[] public array;
event SafeReceived(address indexed sender, uint256 value);

constructor() {
array.push(0);
}

/// @dev Fallback function accepts ether transactions and set non-zero value to a zero value slot.
receive() external payable {
array.push(block.number);
emit SafeReceived(msg.sender, msg.value);
}
}

contract MockTransfer is RONTransferHelper {
uint256 public track;

constructor() payable {}

function fooTransfer(
address payable _recipient,
uint256 _amount,
uint256 _gas
) external {
if (_unsafeSendRON(_recipient, _amount, _gas)) {
track++;
}
}
}
9 changes: 5 additions & 4 deletions contracts/ronin/staking/CandidateStaking.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@

pragma solidity ^0.8.9;

import "../../extensions/consumers/GlobalConfigConsumer.sol";
import "../../extensions/consumers/PercentageConsumer.sol";
import "../../libraries/AddressArrayUtils.sol";
import "../../interfaces/staking/ICandidateStaking.sol";
import "./BaseStaking.sol";

abstract contract CandidateStaking is BaseStaking, ICandidateStaking, PercentageConsumer {
abstract contract CandidateStaking is BaseStaking, ICandidateStaking, GlobalConfigConsumer, PercentageConsumer {
/// @dev The minimum threshold for being a validator candidate.
uint256 internal _minValidatorStakingAmount;

Expand Down Expand Up @@ -111,15 +112,15 @@ abstract contract CandidateStaking is BaseStaking, ICandidateStaking, Percentage
uint256 _deductingAmount = _pool.stakingAmount;
if (_deductingAmount > 0) {
_deductStakingAmount(_pool, _deductingAmount);
if (!_unsafeSendRON(payable(_pool.admin), _deductingAmount, 3500)) {
if (!_unsafeSendRON(payable(_pool.admin), _deductingAmount, DEFAULT_ADDITION_GAS)) {
emit StakingAmountTransferFailed(_pool.addr, _pool.admin, _deductingAmount, address(this).balance);
}
}

// Settle the unclaimed reward and transfer to the pool admin.
uint256 _lastRewardAmount = _claimReward(_pools[_i], _pool.admin, _newPeriod);
if (_lastRewardAmount > 0) {
_unsafeSendRON(payable(_pool.admin), _lastRewardAmount, 3500);
_unsafeSendRON(payable(_pool.admin), _lastRewardAmount, DEFAULT_ADDITION_GAS);
}
}

Expand Down Expand Up @@ -149,7 +150,7 @@ abstract contract CandidateStaking is BaseStaking, ICandidateStaking, Percentage
if (_remainAmount < _minValidatorStakingAmount) revert ErrStakingAmountLeft();

_unstake(_pool, _requester, _amount);
if (!_unsafeSendRON(payable(_requester), _amount, 3500)) revert ErrCannotTransferRON();
if (!_unsafeSendRON(payable(_requester), _amount, DEFAULT_ADDITION_GAS)) revert ErrCannotTransferRON();
}

/**
Expand Down
11 changes: 8 additions & 3 deletions contracts/ronin/staking/Staking.sol
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,14 @@ contract Staking is IStaking, CandidateStaking, DelegatorStaking, Initializable
returns (uint256 _actualDeductingAmount)
{
_actualDeductingAmount = _deductStakingAmount(_stakingPool[_consensusAddr], _amount);
address payable _recipientAddr = payable(validatorContract());
if (!_unsafeSendRON(_recipientAddr, _actualDeductingAmount, 3500)) {
emit StakingAmountDeductFailed(_consensusAddr, _recipientAddr, _actualDeductingAmount, address(this).balance);
address payable _validatorContractAddr = payable(validatorContract());
if (!_unsafeSendRON(_validatorContractAddr, _actualDeductingAmount)) {
emit StakingAmountDeductFailed(
_consensusAddr,
_validatorContractAddr,
_actualDeductingAmount,
address(this).balance
);
}
}

Expand Down
5 changes: 3 additions & 2 deletions contracts/ronin/validator/CandidateManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
pragma solidity ^0.8.9;

import "../../extensions/collections/HasStakingContract.sol";
import "../../extensions/consumers/GlobalConfigConsumer.sol";
import "../../extensions/consumers/PercentageConsumer.sol";
import "../../interfaces/validator/ICandidateManager.sol";
import "../../interfaces/staking/IStaking.sol";

abstract contract CandidateManager is ICandidateManager, PercentageConsumer, HasStakingContract {
abstract contract CandidateManager is ICandidateManager, PercentageConsumer, GlobalConfigConsumer, HasStakingContract {
/// @dev Maximum number of validator candidate
uint256 private _maxValidatorCandidate;

Expand Down Expand Up @@ -124,7 +125,7 @@ abstract contract CandidateManager is ICandidateManager, PercentageConsumer, Has
if (_effectiveDaysOnwards < _minEffectiveDaysOnwards) revert ErrInvalidEffectiveDaysOnwards();

CommissionSchedule storage _schedule = _candidateCommissionChangeSchedule[_consensusAddr];
uint256 _effectiveTimestamp = ((block.timestamp / 1 days) + _effectiveDaysOnwards) * 1 days;
uint256 _effectiveTimestamp = ((block.timestamp / PERIOD_DURATION) + _effectiveDaysOnwards) * PERIOD_DURATION;
_schedule.effectiveTimestamp = _effectiveTimestamp;
_schedule.commissionRate = _commissionRate;

Expand Down
4 changes: 2 additions & 2 deletions contracts/ronin/validator/CoinbaseExecution.sol
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ abstract contract CoinbaseExecution is
function _distributeMiningReward(address _consensusAddr, address payable _treasury) private {
uint256 _amount = _miningReward[_consensusAddr];
if (_amount > 0) {
if (_unsafeSendRON(_treasury, _amount, 3500)) {
if (_unsafeSendRON(_treasury, _amount, DEFAULT_ADDITION_GAS)) {
emit MiningRewardDistributed(_consensusAddr, _treasury, _amount);
return;
}
Expand All @@ -294,7 +294,7 @@ abstract contract CoinbaseExecution is
) private {
uint256 _amount = _bridgeOperatingReward[_consensusAddr];
if (_amount > 0) {
if (_unsafeSendRON(_treasury, _amount, 3500)) {
if (_unsafeSendRON(_treasury, _amount, DEFAULT_ADDITION_GAS)) {
emit BridgeOperatorRewardDistributed(_consensusAddr, _bridgeOperator, _treasury, _amount);
return;
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/ronin/validator/EmergencyExit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ abstract contract EmergencyExit is IEmergencyExit, RONTransferHelper, CandidateM
_lockedConsensusList.pop();

_lockedFundReleased[_consensusAddr] = true;
if (_unsafeSendRON(_recipient, _amount, 3500)) {
if (_unsafeSendRON(_recipient, _amount, DEFAULT_ADDITION_GAS)) {
emit EmergencyExitLockedFundReleased(_consensusAddr, _recipient, _amount);
return;
}
Expand Down
8 changes: 3 additions & 5 deletions contracts/ronin/validator/storage-fragments/TimingStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@

pragma solidity ^0.8.9;

import "../../../extensions/consumers/GlobalConfigConsumer.sol";
import "../../../interfaces/validator/info-fragments/ITimingInfo.sol";

abstract contract TimingStorage is ITimingInfo {
/// @dev Length of period in seconds
uint256 internal constant _periodLength = 1 days;

abstract contract TimingStorage is ITimingInfo, GlobalConfigConsumer {
/// @dev The number of blocks in a epoch
uint256 internal _numberOfBlocksInEpoch;
/// @dev The last updated block
Expand Down Expand Up @@ -93,6 +91,6 @@ abstract contract TimingStorage is ITimingInfo {
* @dev Returns the calculated period.
*/
function _computePeriod(uint256 _timestamp) internal pure returns (uint256) {
return _timestamp / _periodLength;
return _timestamp / PERIOD_DURATION;
}
}
99 changes: 99 additions & 0 deletions test/transfer/PaymentFallback.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { expect } from 'chai';
import { ethers } from 'hardhat';
import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers';

import {
MockPaymentFallback,
MockPaymentFallbackExpensive,
MockPaymentFallbackExpensive__factory,
MockPaymentFallback__factory,
MockTransfer,
MockTransfer__factory,
} from '../../src/types';
import { BigNumber } from 'ethers';

let senderContract: MockTransfer;
let receiverContract: MockPaymentFallback;
let receiverExpensiveContract: MockPaymentFallbackExpensive;

let deployer: SignerWithAddress;
let signers: SignerWithAddress[];

describe('Payment fallback test', () => {
before(async () => {
[deployer, ...signers] = await ethers.getSigners();
senderContract = await new MockTransfer__factory(deployer).deploy({ value: 1000 });
receiverContract = await new MockPaymentFallback__factory(deployer).deploy();
receiverExpensiveContract = await new MockPaymentFallbackExpensive__factory(deployer).deploy();
});
describe('Receiver contract only emit one event in fallback', async () => {
it('Should transfer successfully with 0 gas in addition', async () => {
let value = 1;
let gas = 0;
await expect(senderContract.fooTransfer(receiverContract.address, value, gas))
.emit(receiverContract, 'SafeReceived')
.withArgs(senderContract.address, value);
});
it('Should transfer successfully with 2300 gas in addition', async () => {
let value = 1;
let gas = 2300;
await expect(senderContract.fooTransfer(receiverContract.address, value, gas))
.emit(receiverContract, 'SafeReceived')
.withArgs(senderContract.address, value);
});
it('Should transfer successfully with 3500 gas in addition', async () => {
let value = 1;
let gas = 3500;
await expect(senderContract.fooTransfer(receiverContract.address, value, gas))
.emit(receiverContract, 'SafeReceived')
.withArgs(senderContract.address, value);
});
});
describe('Receiver contract only emit one event in fallback and set one storage in contract', async () => {
it('Should transfer failed with 0 gas in addition', async () => {
let value = 1;
let gas = 0;
let tx;
await expect(
async () => (tx = await senderContract.fooTransfer(receiverExpensiveContract.address, value, gas))
).changeEtherBalances([receiverExpensiveContract.address], [BigNumber.from(0)]);
await expect(tx).not.emit(receiverExpensiveContract, 'SafeReceived');
});
it('Should transfer failed with 1000 gas in addition', async () => {
let value = 1;
let gas = 1000;
let tx;
await expect(
async () => (tx = await senderContract.fooTransfer(receiverExpensiveContract.address, value, gas))
).changeEtherBalances([receiverExpensiveContract.address], [BigNumber.from(0)]);
await expect(tx).not.emit(receiverExpensiveContract, 'SafeReceived');
});
it('Should transfer failed with 2300 gas in addition', async () => {
let value = 1;
let gas = 2300;
let tx;
await expect(
async () => (tx = await senderContract.fooTransfer(receiverExpensiveContract.address, value, gas))
).changeEtherBalances([receiverExpensiveContract.address], [BigNumber.from(0)]);
await expect(tx).not.emit(receiverExpensiveContract, 'SafeReceived');
});
it('Should transfer failed with 20000 gas in addition', async () => {
let value = 1;
let gas = 20000;
let tx;
await expect(
async () => (tx = await senderContract.fooTransfer(receiverExpensiveContract.address, value, gas))
).changeEtherBalances([receiverExpensiveContract.address], [BigNumber.from(0)]);
await expect(tx).not.emit(receiverExpensiveContract, 'SafeReceived');
});
it('Should transfer successfully with 26000 gas in addition', async () => {
let value = 1;
let gas = 26000;
let tx;
await expect(
async () => (tx = await senderContract.fooTransfer(receiverExpensiveContract.address, value, gas))
).changeEtherBalances([receiverExpensiveContract.address], [BigNumber.from(1)]);
await expect(tx).emit(receiverExpensiveContract, 'SafeReceived');
});
});
});

0 comments on commit 0dd2ec4

Please sign in to comment.