Platform: Code4rena
Start Date: 29/03/2022
Pot Size: $50,000 USDC
Total HM: 16
Participants: 42
Period: 5 days
Judge: 0xean
Total Solo HM: 9
Id: 105
League: ETH
Rank: 4/42
Findings: 2
Award: $4,151.74
🌟 Selected for report: 2
🚀 Solo Findings: 2
🌟 Selected for report: jayjonah8
2075.8711 USDC - $2,075.87
https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/open-zeppelin/ERC20.sol#L149
In HolyPaladinToken.sol it imports ERC20.sol with some changes from the original Open Zeppelin standard. One change is that the transferFrom() function does not follow the Checks Effect and Interactions safety pattern to safely make external calls to other contracts. All checks should be handled first, then any effects/state updates, followed by the external call to prevent reentrancy attacks. Currently the transferFrom() function in ERC20.sol used by HolyPaladinToken.sol calls _transfer() first and then updates the sender allowance which is highly unsafe. The openZeppelin ER20.sol contract which is the industry standard first updates the sender allowance before calling _transfer. The external call should always be done last to avoid any double spending bugs or reentrancy attacks.
https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html
https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/open-zeppelin/ERC20.sol#L149
Open Zeppelins Implementation https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol
Manual code review
Be sure to follow the Checks Effects and Interactions safety pattern as the transferFrom function is one of the most important functions in any protocol. Consider importing the Open Zeppelin ERC20.sol contract code directly as it is battle tested and safe code.
#0 - Kogaroshi
2022-03-31T11:14:37Z
ERC20.sol used was an older version from OpenZeppelin Updated in the codebase via the PR: https://github.com/PaladinFinance/Paladin-Tokenomics/pull/1 We know use the latests version of ERC20.sol that has the correct Checks Effects and Interactions safety pattern
#1 - 0xean
2022-04-08T20:39:11Z
While I agree this is an issue, I think the severity has been overstated slightly. The transferFrom() function itself does not make any external calls so there is no exposure to reentrancy issues. That being said in the future if the contract had been extended in a way that enable an external call this would prove problematic.
Since the sponsor has confirmed the issue and this does violate well known best practices and open up the codebase to future issues, I will award the medium severity.
🌟 Selected for report: jayjonah8
2075.8711 USDC - $2,075.87
https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L25
In HolyPaladinToken.sol the ONE_YEAR variable claims that there are 31557600 seconds in a year when this is incorrect. The ONE_YEAR variable is used in the getCurrentVotes() function as well as the getPastVotes() function so it is vital that the correct time in seconds be used as it can effect users negatively.
https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L25
86,400 seconds in a day x 365 = 31_536_000
Manual code review
The correct number of seconds in a year is 31_536_000 so the ONE_YEAR variable should be changed to ONE_YEAR = 31_536_000
#0 - Kogaroshi
2022-03-31T11:18:27Z
An incorrect value for MONTH was used, leading to all temporal constants (YEAR, max lock time, etc ...) to be incorrect. All values were fixed in: https://github.com/PaladinFinance/Paladin-Tokenomics/pull/3