search

code-423n4 / 2022-03-paladin-findings

0 stars 0 forks source link

Reentrancy #11

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L346

Vulnerability details

Impact

Potential Reentrancy in staking/unstaking function

Proof of Concept

Reentrancy in HolyPaladinToken.stakeAndIncreaseLock() (contracts/HolyPaladinToken.sol#346-365):

    External calls:

    - stakedAmount = _stake(msg.sender,amount) (contracts/HolyPaladinToken.sol#353)

    External calls sending eth:

    - stakedAmount = _stake(msg.sender,amount) (contracts/HolyPaladinToken.sol#353)

    State variables written after the call(s):

    - _delegate(msg.sender,msg.sender) (contracts/HolyPaladinToken.sol#356)

Recommended Mitigation Steps

Use reentrancy guard

Kogaroshi commented 2 years ago

All the external call in the hPAL contract (to stake & unstake) are done to the PAL token, a trusted ERC20 (https://github.com/PaladinFinance/Paladin-Tokenomics/blob/main/contracts/PaladinToken.sol)

0xean commented 2 years ago

I am closing as invalid due to the fact that these external calls are to the PaladinToken. I would still recommend that the check-effects-interactions pattern be followed as a matter of best practice if the code changes or is used elsewhere, but at best that is a style issue when calling a token implementation that is "trusted" or written in house.