Platform: Code4rena
Start Date: 23/02/2024
Pot Size: $92,000 USDC
Total HM: 0
Participants: 47
Period: 10 days
Judge: 0xTheC0der
Id: 336
League: ETH
Rank: 13/47
Findings: 2
Award: $716.32
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: CodeWasp
Also found by: 0xdice91, 0xlemon, Aamir, Al-Qa-qa, AlexCzm, BAHOZ, Bauchibred, Breeje, DadeKuma, Fassi_Security, PetarTolev, Shield, SpicyMeatball, Trust, ZanyBonzy, cheatc0d3, gesha17, haxatron, imare, jesjupyter, kutugu, lsaudit, marchev, merlinboii, nnez, osmanozdemir1, peanuts, radev_sw, twicek, visualbits
694.2987 USDC - $694.30
Links to affected code *
https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/UniStaker.sol#L651 https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/UniStaker.sol#L753 https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/UniStaker.sol#L232-L233C
There's no limit minimum or otherwise on the how much can be staked by users. A user(or his beneficiary) who stakes when totalStaked
is 0, and stakes only 1 wei, will be given full rewards distributable for that period, which will be significantly more than 1 wei. This is because staking with 1 wei can inflate the rewardPerTokenAccumulatedCheckpoint
.
When the user stakes 1 wei into the empty pool, the total amount staked is updated from 0 to 1 wei.
function _stake(address _depositor, uint256 _amount, address _delegatee, address _beneficiary) internal returns (DepositIdentifier _depositId) { ... totalStaked += _amount; ... } https://github.com/sherlock-audit/2023-12-truflation-judging/issues/130
The GlobalReward is checkpointed through the _checkpointGlobalReward
function which makes a call to the rewardPerTokenAccumulated
function. The purpose of this is to calculate the rewardPerTokenAccumulatedCheckpoint
function _checkpointGlobalReward() internal { rewardPerTokenAccumulatedCheckpoint = rewardPerTokenAccumulated(); lastCheckpointTime = lastTimeRewardDistributed(); }
After this is calculated, the beneificiary's rewards is then checkpointed through the _checkpointReward
function which sets the rewardPerTokenAccumulatedCheckpoint
to the beneficiary.
function _checkpointReward(address _beneficiary) internal { unclaimedRewardCheckpoint[_beneficiary] = unclaimedReward(_beneficiary); beneficiaryRewardPerTokenCheckpoint[_beneficiary] = rewardPerTokenAccumulatedCheckpoint; }
The rewardPerTokenAccumulated
function is calculated using the formula, which is can essentially be boiled down to rate / totalstaked, which by dividing by 1 wei, will result in a massive rewardPerTokenAccumulated
being returned.
function rewardPerTokenAccumulated() public view returns (uint256) { if (totalStaked == 0) return rewardPerTokenAccumulatedCheckpoint; return rewardPerTokenAccumulatedCheckpoint + (scaledRewardRate * (lastTimeRewardDistributed() - lastCheckpointTime)) / totalStaked; }
Upon checking the unclaimed rewards, the user's unclaimed rewards will be equal to rate * user balance / totalstaked which will be the total rate.
function unclaimedReward(address _beneficiary) public view returns (uint256) { return unclaimedRewardCheckpoint[_beneficiary] + ( earningPower[_beneficiary] * (rewardPerTokenAccumulated() - beneficiaryRewardPerTokenCheckpoint[_beneficiary]) ) / SCALE_FACTOR; }
This causes that for just 1 wei of UNI tokens, a the user can claim the entire payout amount. An edge case, but serious one at that.
Introduce a minimum amount of tokens that can be staked.
Lines of code*
After signing a signature, a user might want to cancel it for some reason. Ordinarily, this can be done by using the nonce, but the protocol inherits from OpenZeppelin's Nonces contract, which has the _useNonce
function internal, hence there are no ways to cancel the signature before a deadline. Consequently, the user can't cancel the signature.
Include a function callable only by the position owners to access the internal _useNonce
function so that they can easily cancel the signature if they so desire.
DelegationSurrogate
contractLines of code*
To deploy a DelegationSurrogate contract, the CREATE function is used. The issue is that its vulnerable to Ethereum network reorgs which are rare but still occur. The funds meant to be transferred to a surrogate would be deposited to another surrogate address if they get deployed really close to eachother during the reorg process.
function _fetchOrDeploySurrogate(address _delegatee) internal returns (DelegationSurrogate _surrogate) { _surrogate = surrogates[_delegatee]; if (address(_surrogate) == address(0)) { _surrogate = new DelegationSurrogate(STAKE_TOKEN, _delegatee); surrogates[_delegatee] = _surrogate; emit SurrogateDeployed(_delegatee, address(_surrogate)); } }
Use CREATE2/CREATE 3 with salt.
Lines of code* https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/DelegationSurrogate.sol https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/UniStaker.sol https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/V3FactoryOwner.sol
Any tokens sent to the V3FactoryOwner
, Unistaker
and DelegationSurrogate
can potentially be lost as there's no ways to retrieve them. This can also help to preserve the one of the main invariant in the delegate surrogate contract which can be broken by tokens including governance tokens being sent directly to the surrogate contract.
The sum of the governance token balances of all the surrogate contracts is equal to the sum of all deposits less the sum of all withdrawals."
Introduce an admin controlled recoverERC20
function, that can be used to skim extra tokens.
Lines of code*
https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/UniStaker.sol#L301 https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/UniStaker.sol#L371
The permitAndStake
and permitAndStakeMore
allows users to use the ERC20 permit functionality to approve and stake in one seamless transaction. This is subject to permit frontrunning which can be used to grief users.
function permitAndStake( uint256 _amount, address _delegatee, address _beneficiary, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s ) external returns (DepositIdentifier _depositId) { STAKE_TOKEN.permit(msg.sender, address(this), _amount, _deadline, _v, _r, _s); _depositId = _stake(msg.sender, _amount, _delegatee, _beneficiary); }
function permitAndStakeMore( DepositIdentifier _depositId, uint256 _amount, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s ) external { Deposit storage deposit = deposits[_depositId]; _revertIfNotDepositOwner(deposit, msg.sender); STAKE_TOKEN.permit(msg.sender, address(this), _amount, _deadline, _v, _r, _s); _stakeMore(deposit, _depositId, _amount); }
A potential workaround for this issue can be found here and it involves wrapping the permit
and approve
functions in a try-catch block, to prevent failure.
UNI
token doesn't consider type(uint256).max
as an infinite approvalLines of code* https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/DelegationSurrogate.sol#L27 https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/UniStaker.sol#L605C1-L616C4
Upon DelegationSurrogate
deployment by the Unistaker
contract, the type(uint256).max
approval is given to the Unistaker
.
constructor(IERC20Delegates _token, address _delegatee) { _token.delegate(_delegatee); _token.approve(msg.sender, type(uint256).max); }
The issue is that some tokens including the protocol's own stake token, UNI
downcast type(uint256).max
approvals to uint96
and use it as a raw value rather than interpreting it as an infinite approval. Due to the continuous stacks of calls and safeTransferFrom
s made to the DelegationSurrogate
contract to transfer these tokens upon staking, unstaking, delegation changes and so on, a time will eventually arrive when this allowance will become less than than the amount to transfer, or will reach 0. At, this point, token transfer from the DelegationSurrogate
contract will no longer succeed.
Considering that the DelegationSurrogate
contract is a dead contract and tokens can't be accessed directly from it, the tokens left in it will be lost forever, inaccesible, leading to loss of funds for the user and inability to change delegates. An extreme edge case, but can be devastating.
Introduce an admin controlled approve
function that can be used to continously renew the Unistaker
's approval or a recover tokens function.
A check regarding whether the current value and the new value are the same should be added.
Lines of code* https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/V3FactoryOwner.sol#L110-L124
function setAdmin(address _newAdmin) external { _revertIfNotAdmin(); if (_newAdmin == address(0)) revert V3FactoryOwner__InvalidAddress(); emit AdminSet(admin, _newAdmin); admin = _newAdmin; } /// @notice Update the payout amount to a new value. Must be called by admin. /// @param _newPayoutAmount The value that will be the new payout amount. function setPayoutAmount(uint256 _newPayoutAmount) external { _revertIfNotAdmin(); if (_newPayoutAmount == 0) revert V3FactoryOwner__InvalidPayoutAmount(); emit PayoutAmountSet(payoutAmount, _newPayoutAmount); payoutAmount = _newPayoutAmount; }
function _alterDelegatee( Deposit storage deposit, DepositIdentifier _depositId, address _newDelegatee ) internal { _revertIfAddressZero(_newDelegatee); DelegationSurrogate _oldSurrogate = surrogates[deposit.delegatee]; emit DelegateeAltered(_depositId, deposit.delegatee, _newDelegatee); deposit.delegatee = _newDelegatee; DelegationSurrogate _newSurrogate = _fetchOrDeploySurrogate(_newDelegatee); _stakeTokenSafeTransferFrom(address(_oldSurrogate), address(_newSurrogate), deposit.balance); } /// @notice Internal convenience method which alters the beneficiary of an existing deposit. /// @dev This method must only be called after proper authorization has been completed. /// @dev See public alterBeneficiary methods for additional documentation. function _alterBeneficiary( Deposit storage deposit, DepositIdentifier _depositId, address _newBeneficiary ) internal { _revertIfAddressZero(_newBeneficiary); _checkpointGlobalReward(); _checkpointReward(deposit.beneficiary); earningPower[deposit.beneficiary] -= deposit.balance; _checkpointReward(_newBeneficiary); emit BeneficiaryAltered(_depositId, deposit.beneficiary, _newBeneficiary); deposit.beneficiary = _newBeneficiary; earningPower[_newBeneficiary] += deposit.balance; }
https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/V3FactoryOwner.sol#L119C1-L125C1 https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/V3FactoryOwner.sol#L110C1-L115C4
function setAdmin(address _newAdmin) external { _revertIfNotAdmin(); if (_newAdmin == address(0)) revert V3FactoryOwner__InvalidAddress(); emit AdminSet(admin, _newAdmin); admin = _newAdmin; } /// @notice Update the payout amount to a new value. Must be called by admin. /// @param _newPayoutAmount The value that will be the new payout amount. function setPayoutAmount(uint256 _newPayoutAmount) external { _revertIfNotAdmin(); if (_newPayoutAmount == 0) revert V3FactoryOwner__InvalidPayoutAmount(); emit PayoutAmountSet(payoutAmount, _newPayoutAmount); payoutAmount = _newPayoutAmount; }
name
function. it should include the name function as a basic erc20 implementation, and as a possible interface to the UNI token contract.Lines of code*
interface IERC20Delegates { // ERC20 related methods function allowance(address account, address spender) external view returns (uint256); function approve(address spender, uint256 rawAmount) external returns (bool); function balanceOf(address account) external view returns (uint256); function decimals() external view returns (uint8); function symbol() external view returns (string memory); function totalSupply() external view returns (uint256); function transfer(address dst, uint256 rawAmount) external returns (bool); function transferFrom(address src, address dst, uint256 rawAmount) external returns (bool); function permit( address owner, address spender, uint256 rawAmount, uint256 deadline, uint8 v, bytes32 r, bytes32 s ) external;
#0 - wildmolasses
2024-03-12T16:56:46Z
almost disputable, but giving warden benefit of doubt here because of (4). Not marking as high quality.
#1 - c4-sponsor
2024-03-12T16:56:55Z
wildmolasses (sponsor) acknowledged
#2 - c4-judge
2024-03-14T14:11:42Z
MarioPoneder marked the issue as grade-b
🌟 Selected for report: roguereggiant
Also found by: 0xepley, Aamir, Al-Qa-qa, LinKenji, MSK, McToady, Myd, SAQ, Sathish9098, ZanyBonzy, aariiif, cudo, emerald7017, fouzantanveer, hassanshakeel13, hunter_w3b, ihtishamsudo, kaveyjoe, peanuts
22.023 USDC - $22.02
Scope and Architecture Overview
Phase 1: Protocol Familiarization: The review began by analyzing the protocol documentation, including the readMe, AthleaLI blog and whitepaper. followed by a preliminary examination of the relevant contracts and their tests.
Phase 2: Deep Contract Review: A meticulous, line-by-line review of the in-scope contracts was conducted, meticulously following the expected interaction flow.
Phase 3: Issue Discussion and Resolution: Potential pitfalls identified during the review were discussed with the developers to clarify whether they were genuine issues or intentional features of the protocol.
Phase 4: Reporting: Audit reports were finalized based on the findings.
Creating and increasing staking position - Users create a new staking position by calling the stake
function entering in their desired beneficiary and delegatee addresses. Prior approval must have been given to the Unistaker contract in a two step transactional method. To combine the approval and stake process into one step, the permitAndStake
function can be called which uses the token's permit functionality to allow for direct staking using a signature. Also, users can allow other users to open staking positions for them by calling the stakeOnBehalf
function. Users can also increase their current staking positions through the stakeMore
, permitAndStakeMore
functions, or have someone do it for them by calling the stakeMoreOnBehalf
functions which allows for increasing the total amount staked by the user.
Surrogate deployment - When users stake, they're required to provide an address to which their voting power is delegated to. Doing this requires that the delegatee has a surrogate contract, which functions as an escrow for the user's tokens. The _fetchOrDeploySurrogate
function attampts to find a delegatee's surrogate contract, and if its non existent, a new one is deployed using the CREATE method.
Reducing staking position - Users can take away some of all of their tokens from stake by calling the withdraw
function, which essentially behaves like a typical unstake
function would. The amount to unstake is withdrawn from the position and tokens transferred to the user. This can also be done on behalf of the user by calling the withdrawOnBehalf
function. Important to note that staking positions are not deleted even after all tokens have been unstaked. The position is held for users to fill up by calling the functions to stake more.
Claiming staking rewards - Beneficiaries of staking positions call the claimReward
which calculates and send out the rewards that they're entitled to. They can allow other users to claim on the behalf using their signature through the claimRewardOnBehalf
function.
Changing beneficiary and delegates - Upon staking, users specify the address to get rewards from their staking i.e the beneficiary, and address to get their voting power, the delegate. These addresses can be changed by calling the alterBeneficiary
and the alterDelegatee
functions. The alterBeneficiary
sets the new beneficiary - Important to note that the previous beneficiaries do not lose their previous rewards, a checkpoint is set for them. The alterDelegatee
transfers the voting power from the previous delegate to the new delegate. This can also be done for a user through the alterBeneficiaryOnBehalf
and the alterDelegateeOnBehalf
functions.
Reward notification - Being a fork of Synthetix staking contract, the contract implements the notifyRewardAmount
function which alerts it that a new reward has been transfered. Before this function is invoked, the payout amount is sent first, so as to prevent issues when claiming rewards.
<p align="center"> UniStaker.sol; sLOC - 423 </p>UniStaker | |---> Constructor | | | |---> _setAdmin | |---> setAdmin | | | |---> _revertIfNotAdmin | |---> setRewardNotifier | | | |---> _revertIfNotAdmin | |---> stake | | | |---> _stake | | | |---> _checkpointGlobalReward | |---> _checkpointReward | |---> _fetchOrDeploySurrogate | |---> _useDepositId | |---> SafeERC20.safeTransferFrom | |---> permitAndStake | | | |---> STAKE_TOKEN.permit | |---> _stake | |---> stakeOnBehalf | | | |---> _revertIfSignatureIsNotValidNow | |---> _stake | |---> stakeMore | | | |---> _revertIfNotDepositOwner | |---> _stakeMore | |---> permitAndStakeMore | | | |---> _revertIfNotDepositOwner | |---> STAKE_TOKEN.permit | |---> _stakeMore | |---> stakeMoreOnBehalf | | | |--->> _revertIfNotDepositOwner | |---> _revertIfSignatureIsNotValidNow | |---> _stakeMore | |---> alterDelegatee | | | |--- _revertIfNotDepositOwner | |--- _alterDelegatee | |--- alterDelegateeOnBehalf | | | |--- _revertIfNotDepositOwner | |--- _revertIfSignatureIsNotValidNow | |--- _alterDelegatee | |--- alterBeneficiary | | | |--- _revertIfNotDepositOwner | |--- _alterBeneficiary | |--- alterBeneficiaryOnBehalf | | | |--- _revertIfNotDepositOwner | |--- _revertIfSignatureIsNotValidNow | |--- _alterBeneficiary | |--- withdraw | | | |--- _revertIfNotDepositOwner | |--- _withdraw | |--- withdrawOnBehalf | | | |--- _revertIfNotDepositOwner | |--- _revertIfSignatureIsNotValidNow | |--- _withdraw | |--- claimReward | | | |--- _claimReward | |--- claimRewardOnBehalf | | | |--- _revertIfSignatureIsNotValidNow | |--- _claimReward | |--- notifyRewardAmount | | | |--- _revertIfNotRewardNotifier
Unistaker
contract, which sends rewards to it and notifies the contract that a new batch of rewards have been sent.Admin functions - The uniswap governance timelock contract interact with the v3factory and pools through these functions. By calling the enableFeeAmount
, fee amount for a pool is set and activated for collection. The setFeeProtocol
sets the denominator of the protocol's percentage share of the fees. The setPayoutAmount
allows admin to set the amount of the payout token that needs to be paid to claim the pools fees.
Fees claiming - While, arbitraging and race conditions, are mostly viewed in a negative light, the protocol offers a chance to MEV bots and arbitragers to participate in a race to claim a pools fees. The arbitragers have to pay the payout amount for which the fees from activated pools will be sent to them. The payout amount is send to the Unistake
contract and the contract is notified of the payment, upon which rewards are distributed to stakers.
<p align="center"> V3FactoryOwner.sol; sLOC - 87 </p>[Admin] --> |V3FactoryOwner| | |<-- Admin Set | |--> |setAdmin()| | |--> |setPayoutAmount()| | |--> |enableFeeAmount()| | |--> |setFeeProtocol()| | |--> |claimFees()| | | | |--> |IUniswapV3PoolOwnerActions| | | | |--> |IUniswapV3FactoryOwnerActions| | | | |--> |INotifiableRewardReceiver| | | | |--> |IERC20| | |<-- FeesClaimed
<p align="center"> DelegationSurrogate.sol; sLOC - 8 </p>+--------------------------------------------------+ | DelegationSurrogate | +--------------------------------------------------+ | - _token: IERC20Delegates | | - _delegatee: address | +--------------------------------------------------+ | + constructor(_token.delegate, _token.approve) | +--------------------------------------------------+
Audit Information - For the purpose of the security review, the Unistaker codebase consists of three smart contracts totaling 518 SLoC. Its core design principle is inheritance, enabling efficient and flexible integration. It is scheduled to be deployed on Ethereum mainnet, hence subject to its network conditions. It exists as a fork of the Synthetix staking contracts.
Documentation and NatSpec - The provided c4 readme and protcol documentation is top tier. They provide a important overview of the the protocol and its functionality including a breakdown of each contract functions and implementations. The contracts are well commented, each function was well explained. It made the audit process easier.
Error handling and Event Emission - Errors are well handled, custom errors are in use, which among many advantages, is also gas efficient. Events are well handled, emitted for important parameter changes.
Testability - Test coverage is about 100% which is very excellent. The implemented tests include basic unit tests for functions, invariant tests to handle the protocol's main invariants and fuzz tests for the more complicated parts of the codebase.
Token Support - The protocol aims to work with 3 major token types, the unistaker's reward token - WETH, stake token - UNI and the V3FactoryOwner's payout token - WETH. Ideally, any token type can be set by the admin, upon deployment which includes all token types. However, it's important to note that the contracts' implementations are not suitable for most non-standard ERC20 tokens.
Attack Vectors - Points of attack in this contract include gaming rewards distribution, stealing other user's rewards, griefing users' claims and so on. Other areas of attack are vulnerabilities common to synthetix forks and breaking the protocol's main invariants.
Ideally, the admin functions in the contracts are controlled by uniswap's governance timelock which is controlled by the DAO, so its fair to say there isn't a lot of centralization going on. However, there are ways these admin functions can be used to harm the protocol. Some of them include:
The documentation is still missing information about fees and rewards claims. Recommend bringing it up to date.
Edge cases for users not staking or a user staking very little to manipulate rewards should be considered. A potential fix is allowing a minimum stake amount.
Reconsider the decision to have a general payout amount. Due to the difference in pools' liquidity and activity, certain pools might get their fees claimed too often, while some may not have fees claimed at all. A possible fix can be making the payout amounts pool specific.
A bit of refactoring should be made around the transfer functions and accounting to factor in payout tokens that may not fit the typical ERC20 standard, e.g fee-on-transfer tokens, rebasing tokens etc in case supporrt is offered for them later on.
The deployed DelegationSurrogate contract can be beefed up with CREATE2 or CREATE3 to protect from ethereum network conditions like reorgs, forks etc.
A proper pause function should be implemented to protect users during unstable situations. Important to note that only user entry methods should be pausable.
Two step variable and address updates should be implemented. Most of the setter functions implement the changes almost immediately which can be jarring for the contracts/users. Adding these fixes can help protect from admin mistakes and unexepected behaviour.
A sweep function can also be considered for the contracts to carefully retrieve excess tokens sent to them to prevent them from getting lost forever.
In conclusion, the codebase is probably one of the best-written ever audited on C4, which is quite commendable. Howevever, as is the reason for the audit, the identified risks need to be mitigated. Provided recommendations should be considered and implemented or if not implemented, then documented for user awareness.
32 hours
#0 - c4-judge
2024-03-14T18:25:26Z
MarioPoneder marked the issue as grade-b