Skip to content

Commit

Permalink
refactor and test: address internal audit
Browse files Browse the repository at this point in the history
  • Loading branch information
kupermind committed May 17, 2024
1 parent e7ff9a5 commit b73c9c1
Show file tree
Hide file tree
Showing 18 changed files with 162 additions and 69 deletions.
16 changes: 15 additions & 1 deletion .gitleaksignore
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,18 @@ b8377500c8b4a919e7385f4f7000ad4630608767:scripts/deployment/staking/globals_sepo
2461436a81f94372e9732a64b84e3dec78d1c46f:scripts/deployment/staking/globals_sepolia.json:generic-api-key:2
2461436a81f94372e9732a64b84e3dec78d1c46f:scripts/deployment/staking/wormhole/globals_polygon_mainnet.json:generic-api-key:2
2461436a81f94372e9732a64b84e3dec78d1c46f:scripts/deployment/staking/wormhole/globals_celo_mainnet.json:generic-api-key:2
7ebe336abdbeb0df14fe17aabe92039c71c52af4:scripts/deployment/staking/test/arbitrum/bridge_token.js:generic-api-key:34
7ebe336abdbeb0df14fe17aabe92039c71c52af4:scripts/deployment/staking/test/arbitrum/bridge_token.js:generic-api-key:34
1735b0835b51076bd531bfe44bd64d13da7390eb:audits/internal4/analysis/contracts/TokenomicsConstants-flatten.sol:generic-api-key:19
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:1826
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:4728
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:4729
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:17130
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:17130
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:17130
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:17132
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:17132
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:17132
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:17208
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:18030
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:19074
c1ebdf6ee1d58b2a7aa8ece47be8aaf70e4768ec:audits/internal4/analysis/contracts/diff-v1.0.3-v1.2.0-pre-internal-audit.txt:generic-api-key:19082
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,6 @@
[submodule "lib/zuniswapv2"]
path = lib/zuniswapv2
url = [email protected]:Jeiwan/zuniswapv2.git
[submodule "lib/fx-portal"]
path = lib/fx-portal
url = https://github.com/0xPolygon/fx-portal
26 changes: 26 additions & 0 deletions audits/internal4/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,32 +43,45 @@ contract Dispenser {
/// @param _voteWeighting Vote Weighting address.
constructor(address _tokenomics, address _treasury, address _voteWeighting) {}
```
[x] fixed

2. checking by possibility `epochAfterRemoved - 1` < 0 in revert message
```
contract Dispenser:
revert Overflow(firstClaimedEpoch, epochAfterRemoved - 1);
```
[x] fixed

3. checking transferAmounts > 0 before transfer
```
contract Dispenser:
in any IToken(olas).transfer(depositProcessor, transferAmounts[i]) or IToken(olas).transfer(depositProcessor, transferAmount); check transferAmount > 0
```
[x] fixed

4. Double check calculateStakingIncentives. Let discussed.
```
Need to rewrite the code such that anybody is able to call the calculateStakingIncentives function, without a possibility to write into the storage to arbitrary caller.
```
[x] fixed

5. sync pause of Dispenser with pause of Treasury
```
The Treasury contract must be leading and if Treasury has set a pause, then it must be checked on Dispeser side and pause too.
```
[x] fixed

6. doublke check function retain() external
```
function looks like she doesn't actually do anything at the end.
totalReturnAmount /= 1e18;
/// action??? with totalReturnAmount
}
```
[x] fixed

7. removeNominee issue around end-of-epoch. revert if `remove` near `end-of-epoch - 7days`. Let discussed.
[x] fixed

##### Other
1. Bug in polygon? Anybody after deploy contract can setup fxChildTunnel. Issue? + lacks a zero-check on
Expand Down Expand Up @@ -136,6 +149,7 @@ abstract contract FxBaseRootTunnel {
fxRootTunnel = _fxRootTunnel;
}
```
[x] fixed

#### Low priority issue
1. does not emit an event
Expand All @@ -146,6 +160,8 @@ PolygonDepositProcessorL1.setL2TargetDispenser()
Dispenser.setPause()
Dispenser.changeStakingParams()
```
[x] fixed

2. abi.encodeWithSignature to abi.encodeCall
```
Example of more safe way:
Expand All @@ -156,6 +172,8 @@ Example of more safe way:
(bool success, bytes memory result) = target.call(data);
ref: https://detectors.auditbase.com/abiencodecall-over-signature-solidity
```
[x] fixed

3. lacks a zero-check on
```
contract ArbitrumTargetDispenserL2 is DefaultTargetDispenserL2 {
Expand Down Expand Up @@ -186,11 +204,15 @@ contract ArbitrumTargetDispenserL2 is DefaultTargetDispenserL2 {
l1AliasedDepositProcessor = address(uint160(_l1DepositProcessor) + offset);
}
```
[x] already implemented in the DefaultTargetDispenserL2 constructor

4. Better add _lock for retain, Because it's impossible to write in CEI-forms
```
Dispenser:
function retain() external {}
```
[x] fixed

5. Low probability overflow. Pay attention to all operation: a += b, type(a) => uint96
```
function refundFromStaking(uint256 amount) external {
Expand All @@ -210,6 +232,7 @@ to
revert Overflow(eBond, type(uint96).max);
}
```
[x] already implemented in a newer version

#### To Discussion
1. Mutex for _processData.
Expand All @@ -224,6 +247,8 @@ function _processData(bytes memory data) internal {
_locked = 1;
}
```
[x] fixed

2. A lot of warnings - "ignores return value".
```
TokenSender.transferTokens(address,uint256,uint16,address,bytes) (WormholeTargetDispenserL2-flatten.sol#1703-1727) ignores return value by IERC20(token).approve(address(tokenBridge),amount) (WormholeTargetDispenserL2-flatten.sol#1710)
Expand Down Expand Up @@ -272,3 +297,4 @@ DefaultTargetDispenserL2._processData(bytes) (PolygonTargetDispenserL2-flatten.s
DefaultTargetDispenserL2.redeem(address,uint256,uint256) (PolygonTargetDispenserL2-flatten.sol#314-351) ignores return value by IToken(olas).approve(target,amount) (PolygonTargetDispenserL2-flatten.sol#338)
-
```
[x] noted
90 changes: 42 additions & 48 deletions contracts/Dispenser.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
pragma solidity ^0.8.25;

// Deposit Processor interface
interface IDepositProcessor {
Expand Down Expand Up @@ -244,24 +244,26 @@ error WrongAccount(bytes32 account);
/// @author Andrey Lebedev - <[email protected]>
/// @author Mariapia Moscatiello - <[email protected]>
contract Dispenser {
enum Pause {
Unpaused,
DevIncentivesPaused,
StakingIncentivesPaused,
AllPaused
}

event OwnerUpdated(address indexed owner);
event TokenomicsUpdated(address indexed tokenomics);
event TreasuryUpdated(address indexed treasury);
event VoteWeightingUpdated(address indexed voteWeighting);
event RetainerUpdated(bytes32 indexed retainer);
event StakingParamsUpdated(uint256 maxNumClaimingEpochs, uint256 maxNumStakingTargets);
event IncentivesClaimed(address indexed owner, uint256 reward, uint256 topUp);
event StakingIncentivesClaimed(address indexed account, uint256 stakingAmount, uint256 transferAmount,
uint256 returnAmount);
event Retained(address indexed account, uint256 returnAmount);
event SetDepositProcessorChainIds(address[] depositProcessors, uint256[] chainIds);
event WithheldAmountSynced(uint256 chainId, uint256 amount, uint256 updatedWithheldAmount);

enum Pause {
Unpaused,
DevIncentivesPaused,
StakingIncentivesPaused,
AllPaused
}
event PauseDispenser(Pause pauseState);

// Maximum chain Id as per EVM specs
uint256 public constant MAX_EVM_CHAIN_ID = type(uint64).max / 2 - 36;
Expand Down Expand Up @@ -401,7 +403,9 @@ contract Dispenser {
address depositProcessor = mapChainIdDepositProcessors[chainId];

// Transfer corresponding OLAS amounts to the deposit processor
IToken(olas).transfer(depositProcessor, transferAmount);
if (transferAmount > 0) {
IToken(olas).transfer(depositProcessor, transferAmount);
}

if (chainId <= MAX_EVM_CHAIN_ID) {
address stakingTargetEVM = address(uint160(uint256(stakingTarget)));
Expand Down Expand Up @@ -676,6 +680,13 @@ contract Dispenser {
revert ZeroAddress();
}

// Get the current retainer nominee hash
bytes32 currentRetainer = retainer;
// Checks in case the current retainer has still a nonzero address
if (currentRetainer != 0) {
revert WrongAccount(currentRetainer);
}

// Check if the new retainer exists as a nominee
IVoteWeighting.Nominee memory nominee = IVoteWeighting.Nominee(newRetainer, block.chainid);
bytes32 nomineeHash = keccak256(abi.encode(nominee));
Expand All @@ -684,35 +695,6 @@ contract Dispenser {
revert ZeroValue();
}

// Get the current retainer nominee hash
bytes32 currentRetainer = retainer;
// Checks in case the current retainer has still a nonzero address
if (currentRetainer != 0) {
// Get current retainer nominee hash
nominee = IVoteWeighting.Nominee(currentRetainer, block.chainid);
nomineeHash = keccak256(abi.encode(nominee));

// The current retainer must be removed from nominees before switching to a new one
uint256 removedEpochCounter = mapRemovedNomineeEpochs[nomineeHash];
// Check that the retainer is removed
if (removedEpochCounter == 0) {
revert ZeroValue();
}

// Get last retained epoch number
uint256 lastClaimedEpoch = mapLastClaimedStakingEpochs[nomineeHash];
// Note that lastClaimedEpoch must never be zero
if (lastClaimedEpoch == 0) {
revert ZeroValue();
}

// Check that all the funds have been retained for previous epochs
// The retainer must be removed in one of previous epochs
if (removedEpochCounter >= lastClaimedEpoch) {
revert Overflow(removedEpochCounter, lastClaimedEpoch - 1);
}
}

retainer = newRetainer;

emit RetainerUpdated(newRetainer);
Expand All @@ -727,13 +709,15 @@ contract Dispenser {
revert OwnerOnly(msg.sender, owner);
}

if (_maxNumClaimingEpochs > 0) {
maxNumClaimingEpochs = _maxNumClaimingEpochs;
// Check if values are zero
if (_maxNumClaimingEpochs == 0 || _maxNumStakingTargets == 0) {
revert ZeroValue();
}

if (_maxNumStakingTargets > 0) {
maxNumStakingTargets = _maxNumStakingTargets;
}
maxNumClaimingEpochs = _maxNumClaimingEpochs;
maxNumStakingTargets = _maxNumStakingTargets;

emit StakingParamsUpdated(_maxNumClaimingEpochs, _maxNumStakingTargets);
}

/// @dev Sets deposit processor contracts addresses and L2 chain Ids.
Expand Down Expand Up @@ -935,10 +919,8 @@ contract Dispenser {
availableStakingAmount = totalWeightSum;
}

// TODO
// Compare the staking weight
// 100% = 1e18, in order to compare with minStakingWeight we need to bring it to the range of 0 .. 10_000
//if (stakingWeight / 1e14 < uint256(stakingPoint.minStakingWeight))
// 100% = 1e18, in order to compare with minStakingWeight we need to bring it from the range of 0 .. 10_000
if (stakingWeight < uint256(stakingPoint.minStakingWeight) * 1e14) {
// If vote weighting staking weight is lower than the defined threshold - return the staking amount
returnAmount = ((stakingDiff + availableStakingAmount) * stakingWeight) / 1e18;
Expand Down Expand Up @@ -1005,7 +987,8 @@ contract Dispenser {

// Check for the paused state
Pause currentPause = paused;
if (currentPause == Pause.StakingIncentivesPaused || currentPause == Pause.AllPaused) {
if (currentPause == Pause.StakingIncentivesPaused || currentPause == Pause.AllPaused ||
ITreasury(treasury).paused() == 2) {
revert Paused();
}

Expand Down Expand Up @@ -1099,7 +1082,8 @@ contract Dispenser {
}

Pause currentPause = paused;
if (currentPause == Pause.StakingIncentivesPaused || currentPause == Pause.AllPaused) {
if (currentPause == Pause.StakingIncentivesPaused || currentPause == Pause.AllPaused ||
ITreasury(treasury).paused() == 2) {
revert Paused();
}

Expand Down Expand Up @@ -1147,6 +1131,12 @@ contract Dispenser {

/// @dev Retains staking incentives according to the retainer address to return it back to the staking inflation.
function retain() external {
// Reentrancy guard
if (_locked > 1) {
revert ReentrancyGuard();
}
_locked = 2;

// Go over epochs and retain funds to return back to the tokenomics
bytes32 localRetainer = retainer;

Expand Down Expand Up @@ -1197,6 +1187,8 @@ contract Dispenser {
}

emit Retained(msg.sender, totalReturnAmount);

_locked = 1;
}

/// @dev Syncs the withheld amount according to the data received from L2.
Expand Down Expand Up @@ -1277,5 +1269,7 @@ contract Dispenser {
}

paused = pauseState;

emit PauseDispenser(pauseState);
}
}
6 changes: 3 additions & 3 deletions contracts/Tokenomics.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
pragma solidity ^0.8.25;

import {convert, UD60x18} from "@prb/math/src/UD60x18.sol";
import {TokenomicsConstants} from "./TokenomicsConstants.sol";
Expand Down Expand Up @@ -763,8 +763,8 @@ contract Tokenomics is TokenomicsConstants {
revert Overflow(_maxStakingAmount, type(uint96).max);
}

if (_minStakingWeight > 10_000) {
revert Overflow(_minStakingWeight, 10_000);
if (_minStakingWeight > MAX_STAKING_WEIGHT) {
revert Overflow(_minStakingWeight, MAX_STAKING_WEIGHT);
}

// All the adjustments will be accounted for in the next epoch
Expand Down
4 changes: 3 additions & 1 deletion contracts/TokenomicsConstants.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
pragma solidity ^0.8.25;

/// @title TokenomicsConstants - Smart contract with tokenomics constants
/// @author Aleksandr Kuperman - <[email protected]>
Expand All @@ -19,6 +19,8 @@ abstract contract TokenomicsConstants {
uint256 public constant MAX_EPOCH_LENGTH = ONE_YEAR - 1 days;
// Minimum fixed point tokenomics parameters
uint256 public constant MIN_PARAM_VALUE = 1e14;
// Max staking weight amount
uint256 public constant MAX_STAKING_WEIGHT = 10_000;

/// @dev Gets an inflation cap for a specific year.
/// @param numYears Number of years passed from the launch date.
Expand Down
3 changes: 3 additions & 0 deletions contracts/staking/DefaultDepositProcessorL1.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ interface IToken {
abstract contract DefaultDepositProcessorL1 is IBridgeErrors {
event MessagePosted(uint256 indexed sequence, address[] targets, uint256[] stakingAmounts, uint256 transferAmount);
event MessageReceived(address indexed l1Relayer, uint256 indexed chainId, bytes data);
event L2TargetDispenserUpdated(address indexed l2TargetDispenser);

// receiveMessage selector to be executed on L2
bytes4 public constant RECEIVE_MESSAGE = bytes4(keccak256(bytes("receiveMessage(bytes)")));
Expand Down Expand Up @@ -196,6 +197,8 @@ abstract contract DefaultDepositProcessorL1 is IBridgeErrors {

// Revoke the owner role making the contract ownerless
owner = address(0);

emit L2TargetDispenserUpdated(l2Dispenser);
}

/// @dev Sets L2 target dispenser address.
Expand Down
Loading

0 comments on commit b73c9c1

Please sign in to comment.