VTVL contest - Dravee's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L11 https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L25

Vulnerability details

Impact

The owner won't be able to pass the AccessProtected mechanism. Only admins can.

Proof of Concept

• AccessProtected imports OZ's Ownable:

File: AccessProtected.sol
5: import "@openzeppelin/contracts/access/Ownable.sol";

• Several comments suggest that admin and owner roles are different and should both be checked:

File: AccessProtected.sol
8: @title Access Limiter to multiple owner-specified accounts.
9: @dev Exposes the onlyAdmin modifier, which will revert (ADMIN_ACCESS_REQUIRED) if the caller is not the owner nor the admin.

21:     /**
22:      * Throws if called by any account that isn't an admin or an owner.
23:      */
24:     modifier onlyAdmin() {
25:         require(_admins[_msgSender()], "ADMIN_ACCESS_REQUIRED");
26:         _;
27:     }

• However, the owner role is never checked. It's not even inherited by AccessProtected while the library is imported:

File: AccessProtected.sol
11: abstract contract AccessProtected is Context {

Recommended Mitigation Steps

Consider inheriting Ownable and checking the owner:

File: AccessProtected.sol
- 11: abstract contract AccessProtected is Context {
+ 11: abstract contract AccessProtected is Context, Ownable {
...
24:     modifier onlyAdmin() {
- 25:         require(_admins[_msgSender()], "ADMIN_ACCESS_REQUIRED");
+ 25:         require(_admins[_msgSender()] || owner() == _msgSender(), "ADMIN_ACCESS_REQUIRED");
26:         _;
27:     }

Alternatively, if the owner here isn't relevant, consider adding an array of _owners which would be checked the same way as the array of _admins and remove the unused library import.

Alternatively, if admins and owners are the same role, just remove the confusing comments